[linux-cifs-client] Re: lookup intent patch
Jeff Layton
jlayton at redhat.com
Fri Mar 27 18:13:22 GMT 2009
On Fri, 27 Mar 2009 10:15:21 -0500
Shirish Pargaonkar <shirishpargaonkar at gmail.com> wrote:
Please run this through checkpatch.pl, there is a lot of bad whitespace
in this patch:
> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> index f9b6f68..011d0ac 100644
> --- a/fs/cifs/dir.c
> +++ b/fs/cifs/dir.c
> @@ -129,12 +129,67 @@ cifs_bp_rename_retry:
> return full_path;
> }
>
> +static void
> +cifs_fill_fileinfo(struct inode *newinode, __u16 fileHandle,
> + struct cifsTconInfo *tcon, bool write_only)
> +{
> + int oplock = 0;
> + struct cifsFileInfo *pCifsFile;
> + struct cifsInodeInfo *pCifsInode;
> +
> + cERROR(1, ("cifs_fill_fileinfo enter\n"));
> +
> + pCifsFile = kzalloc(sizeof(struct cifsFileInfo), GFP_KERNEL);
> +
> + if (pCifsFile == NULL)
> + return;
> +
> + if (oplockEnabled)
> + oplock = REQ_OPLOCK;
> +
> + pCifsFile->netfid = fileHandle;
> + pCifsFile->pid = current->tgid;
> + pCifsFile->pInode = newinode;
> + pCifsFile->invalidHandle = false;
> + pCifsFile->closePend = false;
> + init_MUTEX(&pCifsFile->fh_sem);
^^^^
Can you convert this to a mutex while you're at it? Might not hurt to make
that a separate patch though.
> + mutex_init(&pCifsFile->lock_mutex);
> + INIT_LIST_HEAD(&pCifsFile->llist);
> + atomic_set(&pCifsFile->wrtPending, 0);
> +
> + /* set the following in open now
> + pCifsFile->pfile = file; */
> + write_lock(&GlobalSMBSeslock);
> + list_add(&pCifsFile->tlist, &tcon->openFileList);
> + pCifsInode = CIFS_I(newinode);
> + if (pCifsInode) {
> + /* if readable file instance put first in list*/
> + if (write_only) {
> + list_add_tail(&pCifsFile->flist,
> + &pCifsInode->openFileList);
> + } else {
> + list_add(&pCifsFile->flist,
> + &pCifsInode->openFileList);
> + }
> + if ((oplock & 0xF) == OPLOCK_EXCLUSIVE) {
> + pCifsInode->clientCanCacheAll = true;
> + pCifsInode->clientCanCacheRead = true;
> + cFYI(1, ("Exclusive Oplock inode %p",
> + newinode));
> + } else if ((oplock & 0xF) == OPLOCK_READ)
> + pCifsInode->clientCanCacheRead = true;
> + }
> + write_unlock(&GlobalSMBSeslock);
> + cERROR(1, ("cifs_fill_fileinfo exit\n"));
> +}
> +
> int cifs_posix_open(char *full_path, struct inode **pinode,
> struct super_block *sb, int mode, int oflags,
> int *poplock, __u16 *pnetfid, int xid)
> {
> int rc;
> __u32 oplock;
> + bool write_only = false;
> FILE_UNIX_BASIC_INFO *presp_data;
> __u32 posix_flags = 0;
> struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
> @@ -172,6 +227,8 @@ int cifs_posix_open(char *full_path, struct inode **pinode,
> if (oflags & O_DIRECT)
> posix_flags |= SMB_O_DIRECT;
>
> + if (!(oflags & FMODE_READ))
> + write_only = true;
>
> rc = CIFSPOSIXCreate(xid, cifs_sb->tcon, posix_flags, mode,
> pnetfid, presp_data, &oplock, full_path,
> @@ -198,6 +255,8 @@ int cifs_posix_open(char *full_path, struct inode **pinode,
>
> posix_fill_in_inode(*pinode, presp_data, 1);
>
> + cifs_fill_fileinfo(*pinode, *pnetfid, cifs_sb->tcon, write_only);
> +
> posix_open_ret:
> kfree(presp_data);
> return rc;
> @@ -239,7 +298,6 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
> char *full_path = NULL;
> FILE_ALL_INFO *buf = NULL;
> struct inode *newinode = NULL;
> - struct cifsInodeInfo *pCifsInode;
> int disposition = FILE_OVERWRITE_IF;
> bool write_only = false;
>
> @@ -410,44 +468,8 @@ cifs_create_set_dentry:
> /* mknod case - do not leave file open */
> CIFSSMBClose(xid, tcon, fileHandle);
> } else if (newinode) {
> - struct cifsFileInfo *pCifsFile =
> - kzalloc(sizeof(struct cifsFileInfo), GFP_KERNEL);
> -
> - if (pCifsFile == NULL)
> - goto cifs_create_out;
> - pCifsFile->netfid = fileHandle;
> - pCifsFile->pid = current->tgid;
> - pCifsFile->pInode = newinode;
> - pCifsFile->invalidHandle = false;
> - pCifsFile->closePend = false;
> - init_MUTEX(&pCifsFile->fh_sem);
> - mutex_init(&pCifsFile->lock_mutex);
> - INIT_LIST_HEAD(&pCifsFile->llist);
> - atomic_set(&pCifsFile->wrtPending, 0);
> -
> - /* set the following in open now
> - pCifsFile->pfile = file; */
> - write_lock(&GlobalSMBSeslock);
> - list_add(&pCifsFile->tlist, &tcon->openFileList);
> - pCifsInode = CIFS_I(newinode);
> - if (pCifsInode) {
> - /* if readable file instance put first in list*/
> - if (write_only) {
> - list_add_tail(&pCifsFile->flist,
> - &pCifsInode->openFileList);
> - } else {
> - list_add(&pCifsFile->flist,
> - &pCifsInode->openFileList);
> - }
> - if ((oplock & 0xF) == OPLOCK_EXCLUSIVE) {
> - pCifsInode->clientCanCacheAll = true;
> - pCifsInode->clientCanCacheRead = true;
> - cFYI(1, ("Exclusive Oplock inode %p",
> - newinode));
> - } else if ((oplock & 0xF) == OPLOCK_READ)
> - pCifsInode->clientCanCacheRead = true;
> - }
> - write_unlock(&GlobalSMBSeslock);
> + cifs_fill_fileinfo(newinode, fileHandle,
> + cifs_sb->tcon, write_only);
> }
> cifs_create_out:
> kfree(buf);
> @@ -580,13 +602,16 @@ int cifs_mknod(struct inode *inode, struct dentry *direntry, int mode,
> return rc;
> }
>
> -
> struct dentry *
> cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
> struct nameidata *nd)
> {
> int xid;
> int rc = 0; /* to get around spurious gcc warning, set to zero here */
> + int oplock = 0;
> + int mode;
> + __u16 fileHandle = 0;
> + bool posix_open = false;
> struct cifs_sb_info *cifs_sb;
> struct cifsTconInfo *pTcon;
> struct inode *newInode = NULL;
> @@ -632,12 +657,26 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
> }
> cFYI(1, ("Full path: %s inode = 0x%p", full_path, direntry->d_inode));
>
> - if (pTcon->unix_ext)
> - rc = cifs_get_inode_info_unix(&newInode, full_path,
> - parent_dir_inode->i_sb, xid);
> - else
> + if (pTcon->unix_ext) {
> + if (!(nd->flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY)) &&
> + (nd->flags & LOOKUP_OPEN)) {
> + if (!((nd->intent.open.flags & O_CREAT ) &&
> + (nd->intent.open.flags & O_EXCL))) {
> + mode = nd->intent.open.create_mode &
> + ~current->fs->umask;
> + rc = cifs_posix_open(full_path, &newInode,
> + parent_dir_inode->i_sb, mode,
> + nd->intent.open.flags, &oplock,
> + &fileHandle, xid);
> + posix_open = true;
> + }
> + }
> + if (!posix_open || ((rc == -EINVAL) || (rc == -EOPNOTSUPP)))
> + rc = cifs_get_inode_info_unix(&newInode, full_path,
> + parent_dir_inode->i_sb, xid);
> + } else
> rc = cifs_get_inode_info(&newInode, full_path, NULL,
> - parent_dir_inode->i_sb, xid, NULL);
> + parent_dir_inode->i_sb, xid, NULL);
>
Ok, so you're opening the file on lookup now. Aren't you supposed to use
lookup_instantiate_filp() to pass an instantiated file pointer back to the
caller? If you don't do this, then I think you're susceptible to leaking
this open file if an error occurs between the lookup and the open.
nfs4 does something very similar to what you're trying to do here. You
might want to look carefully at it as an example. nfs4_atomic_open()
is a good place to start.
> if ((rc == 0) && (newInode != NULL)) {
> if (pTcon->nocase)
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 81747ac..4a7c886 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -284,35 +284,34 @@ int cifs_open(struct inode *inode, struct file *file)
> cifs_sb = CIFS_SB(inode->i_sb);
> tcon = cifs_sb->tcon;
>
> - if (file->f_flags & O_CREAT) {
> - /* search inode for this file and fill in file->private_data */
> - pCifsInode = CIFS_I(file->f_path.dentry->d_inode);
> - read_lock(&GlobalSMBSeslock);
> - list_for_each(tmp, &pCifsInode->openFileList) {
> - pCifsFile = list_entry(tmp, struct cifsFileInfo,
> - flist);
> - if ((pCifsFile->pfile == NULL) &&
> - (pCifsFile->pid == current->tgid)) {
> - /* mode set in cifs_create */
> -
> - /* needed for writepage */
> - pCifsFile->pfile = file;
> -
> - file->private_data = pCifsFile;
> - break;
> - }
> - }
> - read_unlock(&GlobalSMBSeslock);
> - if (file->private_data != NULL) {
> - rc = 0;
> - FreeXid(xid);
> - return rc;
> - } else {
> - if (file->f_flags & O_EXCL)
> - cERROR(1, ("could not find file instance for "
> - "new file %p", file));
> + /* search inode for this file and fill in file->private_data */
> + pCifsInode = CIFS_I(file->f_path.dentry->d_inode);
> + read_lock(&GlobalSMBSeslock);
> + list_for_each(tmp, &pCifsInode->openFileList) {
> + pCifsFile = list_entry(tmp, struct cifsFileInfo,
> + flist);
> + if ((pCifsFile->pfile == NULL) &&
> + (pCifsFile->pid == current->tgid)) {
> + /* mode set in cifs_create */
> +
> + /* needed for writepage */
> + pCifsFile->pfile = file;
> +
> + file->private_data = pCifsFile;
> + break;
> }
> }
> + read_unlock(&GlobalSMBSeslock);
> +
> + if (file->private_data != NULL) {
> + rc = 0;
> + FreeXid(xid);
> + return rc;
> + } else {
> + if ((file->f_flags & O_CREAT) && (file->f_flags & O_EXCL))
> + cERROR(1, ("could not find file instance for "
> + "new file %p", file));
> + }
>
> full_path = build_path_from_dentry(file->f_path.dentry);
> if (full_path == NULL) {
--
Jeff Layton <jlayton at redhat.com>
More information about the linux-cifs-client
mailing list