[linux-cifs-client] Re: DFS patches - progress

[Jeff Layton]

On Tue, 20 May 2008 15:51:34 -0500
"Steve French" <smfrench at gmail.com> wrote:

Looks good overall. Actual changes look fairly small. A lot of the
patch seems to be reduction of indentation and formatting cleanup.
That's a good thing...

> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index 422d4e2..1cf43e1 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -161,6 +161,12 @@ static void cifs_unix_info_to_inode(struct inode *inode,
>  	spin_unlock(&inode->i_lock);
>  }
>  
> +
> +/*
> + *	Needed to setup inode data for the directory which is the
> + *	junction to the new submount (ie to setup the fake directory
> + *      which represents a DFS referral)
> + */
>  static void fill_fake_finddataunix(FILE_UNIX_BASIC_INFO *pfnd_dat,
>  			       struct super_block *sb)
>  {
> @@ -370,11 +376,42 @@ static int get_sfu_mode(struct inode *inode,
>  #endif
>  }
>  
> +/*
> + *	Needed to setup inode data for the directory which is the
> + *	junction to the new submount (ie to setup the fake directory
> + *      which represents a DFS referral)
> + */
> +static void fill_fake_finddata(FILE_ALL_INFO *pfnd_dat,
> +			       struct super_block *sb)
> +{
> +	memset(pfnd_dat, sizeof(FILE_ALL_INFO), 0);
> +
> +/*	__le64 pfnd_dat->AllocationSize = cpu_to_le64(0);
> +	__le64 pfnd_dat->EndOfFile = cpu_to_le64(0);
> +	__u8 pfnd_dat->DeletePending = 0;
> +	__u8 pfnd_data->Directory = 0;
> +	__le32 pfnd_dat->EASize = 0;
> +	__u64 pfnd_dat->IndexNumber = 0;
> +	__u64 pfnd_dat->IndexNumber1 = 0;  */
> +	pfnd_dat->CreationTime =
> +		cpu_to_le64(cifs_UnixTimeToNT(CURRENT_TIME));
> +	pfnd_dat->LastAccessTime =
> +		cpu_to_le64(cifs_UnixTimeToNT(CURRENT_TIME));
> +	pfnd_dat->LastWriteTime =
> +		cpu_to_le64(cifs_UnixTimeToNT(CURRENT_TIME));
> +	pfnd_dat->ChangeTime =
> +		cpu_to_le64(cifs_UnixTimeToNT(CURRENT_TIME));
> +	pfnd_dat->Attributes = cpu_to_le32(ATTR_DIRECTORY);
> +	pfnd_dat->NumberOfLinks = cpu_to_le32(2);
> +}
> +
>  int cifs_get_inode_info(struct inode **pinode,
>  	const unsigned char *full_path, FILE_ALL_INFO *pfindData,
>  	struct super_block *sb, int xid, const __u16 *pfid)
>  {
>  	int rc = 0;
> +	__u32 attr;
> +	struct cifsInodeInfo *cifsInfo;
>  	struct cifsTconInfo *pTcon;
>  	struct inode *inode;
>  	struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
> @@ -399,7 +436,6 @@ int cifs_get_inode_info(struct inode **pinode,
>  			return -ENOMEM;
>  		pfindData = (FILE_ALL_INFO *)buf;
>  
> -try_again_CIFSSMBQPathInfo:
>  		/* could do find first instead but this returns more info */
>  		rc = CIFSSMBQPathInfo(xid, pTcon, full_path, pfindData,
>  			      0 /* not legacy */,
> @@ -417,171 +453,167 @@ try_again_CIFSSMBQPathInfo:
>  		}
>  	}
>  	/* dump_mem("\nQPathInfo return data",&findData, sizeof(findData)); */
> -	if (rc) {
> -		if (rc == -EREMOTE && !is_dfs_referral) {
> -			is_dfs_referral = true;
> -			goto try_again_CIFSSMBQPathInfo;
> -		}
> +	if (rc == -EREMOTE) {
> +		is_dfs_referral = true;
> +		fill_fake_finddata(pfindData, sb);
> +		rc = 0;
> +	} else if (rc)
>  		goto cgii_exit;
> -	} else {
> -		struct cifsInodeInfo *cifsInfo;
> -		__u32 attr = le32_to_cpu(pfindData->Attributes);
>  
> -		/* get new inode */
> +	attr = le32_to_cpu(pfindData->Attributes);
> +
> +	/* get new inode */
> +	if (*pinode == NULL) {
> +		*pinode = new_inode(sb);
>  		if (*pinode == NULL) {
> -			*pinode = new_inode(sb);
> -			if (*pinode == NULL) {
> -				rc = -ENOMEM;
> -				goto cgii_exit;
> -			}
> -			/* Is an i_ino of zero legal? Can we use that to check
> -			   if the server supports returning inode numbers?  Are
> -			   there other sanity checks we can use to ensure that
> -			   the server is really filling in that field? */
> +			rc = -ENOMEM;
> +			goto cgii_exit;
> +		}
> +		/* Is an i_ino of zero legal? Can we use that to check
> +		   if the server supports returning inode numbers?  Are
> +		   there other sanity checks we can use to ensure that
> +		   the server is really filling in that field? */
>  
> -			/* We can not use the IndexNumber field by default from
> -			   Windows or Samba (in ALL_INFO buf) but we can request
> -			   it explicitly.  It may not be unique presumably if
> -			   the server has multiple devices mounted under one
> -			   share */
> +		/* We can not use the IndexNumber field by default from
> +		   Windows or Samba (in ALL_INFO buf) but we can request
> +		   it explicitly.  It may not be unique presumably if
> +		   the server has multiple devices mounted under one share */
>  
> -			/* There may be higher info levels that work but are
> -			   there Windows server or network appliances for which
> -			   IndexNumber field is not guaranteed unique? */
> +		/* There may be higher info levels that work but are
> +		   there Windows server or network appliances for which
> +		   IndexNumber field is not guaranteed unique? */
>  
> -			if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM) {
> -				int rc1 = 0;
> -				__u64 inode_num;
> +		if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM) {
> +			int rc1 = 0;
> +			__u64 inode_num;
>  
> -				rc1 = CIFSGetSrvInodeNumber(xid, pTcon,
> +			rc1 = CIFSGetSrvInodeNumber(xid, pTcon,
>  					full_path, &inode_num,
>  					cifs_sb->local_nls,
>  					cifs_sb->mnt_cifs_flags &
>  						CIFS_MOUNT_MAP_SPECIAL_CHR);
> -				if (rc1) {
> -					cFYI(1, ("GetSrvInodeNum rc %d", rc1));
> -					/* BB EOPNOSUPP disable SERVER_INUM? */
> -				} else /* do we need cast or hash to ino? */
> -					(*pinode)->i_ino = inode_num;
> -			} /* else ino incremented to unique num in new_inode*/
> -			if (sb->s_flags & MS_NOATIME)
> -				(*pinode)->i_flags |= S_NOATIME | S_NOCMTIME;
> -			insert_inode_hash(*pinode);
> -		}
> -		inode = *pinode;
> -		cifsInfo = CIFS_I(inode);
> -		cifsInfo->cifsAttrs = attr;
> -		cFYI(1, ("Old time %ld", cifsInfo->time));
> -		cifsInfo->time = jiffies;
> -		cFYI(1, ("New time %ld", cifsInfo->time));
> -
> -		/* blksize needs to be multiple of two. So safer to default to
> -		blksize and blkbits set in superblock so 2**blkbits and blksize
> -		will match rather than setting to:
> -		(pTcon->ses->server->maxBuf - MAX_CIFS_HDR_SIZE) & 0xFFFFFE00;*/
> -
> -		/* Linux can not store file creation time so ignore it */
> -		if (pfindData->LastAccessTime)
> -			inode->i_atime = cifs_NTtimeToUnix
> -				(le64_to_cpu(pfindData->LastAccessTime));
> -		else /* do not need to use current_fs_time - time not stored */
> -			inode->i_atime = CURRENT_TIME;
> -		inode->i_mtime =
> +			if (rc1) {
> +				cFYI(1, ("GetSrvInodeNum rc %d", rc1));
> +				/* BB EOPNOSUPP disable SERVER_INUM? */
> +			} else /* do we need cast or hash to ino? */
> +				(*pinode)->i_ino = inode_num;
> +		} /* else ino incremented to unique num in new_inode*/

                ^^^^^
...actually this isn't true. There's no guarantee that inode numbers
returned by new_inode are unique. The counter in it can wrap, and you
can get duplicates. If you need unique numbers then you should use
iunique(), but the inodes must all be hashed and it can be a bit slower
if the inode number space is filled.

Still, most apps don't care about i_ino/st_ino. The exception is stuff
like tar or other backup apps that check that to find hardlinks. On a
directory it doesn't much matter, IMO.

> +		if (sb->s_flags & MS_NOATIME)
> +			(*pinode)->i_flags |= S_NOATIME | S_NOCMTIME;
> +		insert_inode_hash(*pinode);
> +	}
> +	inode = *pinode;
> +	cifsInfo = CIFS_I(inode);
> +	cifsInfo->cifsAttrs = attr;
> +	cFYI(1, ("Old time %ld", cifsInfo->time));
> +	cifsInfo->time = jiffies;
> +	cFYI(1, ("New time %ld", cifsInfo->time));
> +
> +	/* blksize needs to be multiple of two. So safer to default to
> +	blksize and blkbits set in superblock so 2**blkbits and blksize
> +	will match rather than setting to:
> +	(pTcon->ses->server->maxBuf - MAX_CIFS_HDR_SIZE) & 0xFFFFFE00;*/
> +
> +	/* Linux can not store file creation time so ignore it */
> +	if (pfindData->LastAccessTime)
> +		inode->i_atime = cifs_NTtimeToUnix
> +			(le64_to_cpu(pfindData->LastAccessTime));
> +	else /* do not need to use current_fs_time - time not stored */
> +		inode->i_atime = CURRENT_TIME;
> +	inode->i_mtime =
>  		    cifs_NTtimeToUnix(le64_to_cpu(pfindData->LastWriteTime));
> -		inode->i_ctime =
> -		    cifs_NTtimeToUnix(le64_to_cpu(pfindData->ChangeTime));
> -		cFYI(0, ("Attributes came in as 0x%x", attr));
> -		if (adjustTZ && (pTcon->ses) && (pTcon->ses->server)) {
> -			inode->i_ctime.tv_sec += pTcon->ses->server->timeAdj;
> -			inode->i_mtime.tv_sec += pTcon->ses->server->timeAdj;
> -		}
> +	inode->i_ctime =
> +	    cifs_NTtimeToUnix(le64_to_cpu(pfindData->ChangeTime));
> +	cFYI(DBG2, ("Attributes came in as 0x%x", attr));
> +	if (adjustTZ && (pTcon->ses) && (pTcon->ses->server)) {
> +		inode->i_ctime.tv_sec += pTcon->ses->server->timeAdj;
> +		inode->i_mtime.tv_sec += pTcon->ses->server->timeAdj;
> +	}
>  
> -		/* set default mode. will override for dirs below */
> -		if (atomic_read(&cifsInfo->inUse) == 0)
> -			/* new inode, can safely set these fields */
> -			inode->i_mode = cifs_sb->mnt_file_mode;
> -		else /* since we set the inode type below we need to mask off
> -		     to avoid strange results if type changes and both
> -		     get orred in */
> -			inode->i_mode &= ~S_IFMT;
> -/*		if (attr & ATTR_REPARSE)  */
> -		/* We no longer handle these as symlinks because we could not
> -		   follow them due to the absolute path with drive letter */
> -		if (attr & ATTR_DIRECTORY) {
> -		/* override default perms since we do not do byte range locking
> -		   on dirs */
> -			inode->i_mode = cifs_sb->mnt_dir_mode;
> -			inode->i_mode |= S_IFDIR;
> -		} else if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_UNX_EMUL) &&
> -			   (cifsInfo->cifsAttrs & ATTR_SYSTEM) &&
> -			   /* No need to le64 convert size of zero */
> -			   (pfindData->EndOfFile == 0)) {
> -			inode->i_mode = cifs_sb->mnt_file_mode;
> -			inode->i_mode |= S_IFIFO;
> +	/* set default mode. will override for dirs below */
> +	if (atomic_read(&cifsInfo->inUse) == 0)
> +		/* new inode, can safely set these fields */
> +		inode->i_mode = cifs_sb->mnt_file_mode;
> +	else /* since we set the inode type below we need to mask off
> +	     to avoid strange results if type changes and both
> +	     get orred in */
> +		inode->i_mode &= ~S_IFMT;
> +/*	if (attr & ATTR_REPARSE)  */
> +	/* We no longer handle these as symlinks because we could not
> +	   follow them due to the absolute path with drive letter */
> +	if (attr & ATTR_DIRECTORY) {
> +	/* override default perms since we do not do byte range locking
> +	   on dirs */
> +		inode->i_mode = cifs_sb->mnt_dir_mode;
> +		inode->i_mode |= S_IFDIR;
> +	} else if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_UNX_EMUL) &&
> +		   (cifsInfo->cifsAttrs & ATTR_SYSTEM) &&
> +		   /* No need to le64 convert size of zero */
> +		   (pfindData->EndOfFile == 0)) {
> +		inode->i_mode = cifs_sb->mnt_file_mode;
> +		inode->i_mode |= S_IFIFO;



>  /* BB Finish for SFU style symlinks and devices */
> -		} else if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_UNX_EMUL) &&
> -			   (cifsInfo->cifsAttrs & ATTR_SYSTEM)) {
> -			if (decode_sfu_inode(inode,
> -					 le64_to_cpu(pfindData->EndOfFile),
> -					 full_path,
> -					 cifs_sb, xid))
> -				cFYI(1, ("Unrecognized sfu inode type"));
> -
> -			cFYI(1, ("sfu mode 0%o", inode->i_mode));
> -		} else {
> -			inode->i_mode |= S_IFREG;
> -			/* treat the dos attribute of read-only as read-only
> -			   mode e.g. 555 */
> -			if (cifsInfo->cifsAttrs & ATTR_READONLY)
> -				inode->i_mode &= ~(S_IWUGO);
> -			else if ((inode->i_mode & S_IWUGO) == 0)
> -				/* the ATTR_READONLY flag may have been	*/
> -				/* changed on server -- set any w bits	*/
> -				/* allowed by mnt_file_mode		*/
> -				inode->i_mode |= (S_IWUGO &
> -						  cifs_sb->mnt_file_mode);
> -		/* BB add code here -
> -		   validate if device or weird share or device type? */
> -		}
> +	} else if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_UNX_EMUL) &&
> +		   (cifsInfo->cifsAttrs & ATTR_SYSTEM)) {
> +		if (decode_sfu_inode(inode, le64_to_cpu(pfindData->EndOfFile),
> +				     full_path, cifs_sb, xid))
> +			cFYI(1, ("Unrecognized sfu inode type"));
>  
> -		spin_lock(&inode->i_lock);
> -		if (is_size_safe_to_change(cifsInfo,
> -					   le64_to_cpu(pfindData->EndOfFile))) {
> -			/* can not safely shrink the file size here if the
> -			   client is writing to it due to potential races */
> -			i_size_write(inode, le64_to_cpu(pfindData->EndOfFile));
> -
> -			/* 512 bytes (2**9) is the fake blocksize that must be
> -			   used for this calculation */
> -			inode->i_blocks = (512 - 1 + le64_to_cpu(
> -					   pfindData->AllocationSize)) >> 9;
> -		}
> -		spin_unlock(&inode->i_lock);
> +		cFYI(1, ("sfu mode 0%o", inode->i_mode));
> +	} else {
> +		inode->i_mode |= S_IFREG;
> +		/* treat dos attribute of read-only as read-only mode eg 555 */
> +		if (cifsInfo->cifsAttrs & ATTR_READONLY)
> +			inode->i_mode &= ~(S_IWUGO);
> +		else if ((inode->i_mode & S_IWUGO) == 0)
> +			/* the ATTR_READONLY flag may have been	*/
> +			/* changed on server -- set any w bits	*/
> +			/* allowed by mnt_file_mode		*/
> +			inode->i_mode |= (S_IWUGO & cifs_sb->mnt_file_mode);
> +	/* BB add code to validate if device or weird share or device type? */
> +	}
> +
> +	spin_lock(&inode->i_lock);
> +	if (is_size_safe_to_change(cifsInfo,
> +				   le64_to_cpu(pfindData->EndOfFile))) {
> +		/* can not safely shrink the file size here if the
> +		   client is writing to it due to potential races */
> +		i_size_write(inode, le64_to_cpu(pfindData->EndOfFile));
> +
> +		/* 512 bytes (2**9) is the fake blocksize that must be
> +		   used for this calculation */
> +		inode->i_blocks = (512 - 1 + le64_to_cpu(
> +				   pfindData->AllocationSize)) >> 9;
> +	}
> +	spin_unlock(&inode->i_lock);
>  
> -		inode->i_nlink = le32_to_cpu(pfindData->NumberOfLinks);
> +	inode->i_nlink = le32_to_cpu(pfindData->NumberOfLinks);
>  
> -		/* BB fill in uid and gid here? with help from winbind?
> -		   or retrieve from NTFS stream extended attribute */
> +	/* BB fill in uid and gid here? with help from winbind?
> +	   or retrieve from NTFS stream extended attribute */
>  #ifdef CONFIG_CIFS_EXPERIMENTAL
> -		/* fill in 0777 bits from ACL */
> -		if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_CIFS_ACL) {
> -			cFYI(1, ("Getting mode bits from ACL"));
> -			acl_to_uid_mode(inode, full_path, pfid);
> -		}
> +	/* fill in 0777 bits from ACL */
> +	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_CIFS_ACL) {
> +		cFYI(1, ("Getting mode bits from ACL"));
> +		acl_to_uid_mode(inode, full_path, pfid);
> +	}
>  #endif
> -		if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_UNX_EMUL) {
> -			/* fill in remaining high mode bits e.g. SUID, VTX */
> -			get_sfu_mode(inode, full_path, cifs_sb, xid);
> -		} else if (atomic_read(&cifsInfo->inUse) == 0) {
> -			inode->i_uid = cifs_sb->mnt_uid;
> -			inode->i_gid = cifs_sb->mnt_gid;
> -			/* set so we do not keep refreshing these fields with
> -			   bad data after user has changed them in memory */
> -			atomic_set(&cifsInfo->inUse, 1);
> -		}
> -
> -		cifs_set_ops(inode, is_dfs_referral);
> +	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_UNX_EMUL) {
> +		/* fill in remaining high mode bits e.g. SUID, VTX */
> +		get_sfu_mode(inode, full_path, cifs_sb, xid);
> +	} else if (atomic_read(&cifsInfo->inUse) == 0) {
> +		inode->i_uid = cifs_sb->mnt_uid;
> +		inode->i_gid = cifs_sb->mnt_gid;
> +		/* set so we do not keep refreshing these fields with
> +		   bad data after user has changed them in memory */
> +		atomic_set(&cifsInfo->inUse, 1);
>  	}
> +
> +	cifs_set_ops(inode, is_dfs_referral);
> +
> +
> +
> +
>  cgii_exit:
>  	kfree(buf);
>  	return rc;
>  

...ugh -- guess I'll need to respin those dynperm patches again. Maybe
I'll wait until the DFS churn settles down. :-)


-- 
Jeff Layton <jlayton at redhat.com>