* Re: lookup intent patch [not found] ` <4a4634330903191336n54758971r2ff809ba31a80791@mail.gmail.com> @ 2009-03-27 15:15 ` Shirish Pargaonkar 2009-03-27 18:13 ` Jeff Layton 0 siblings, 1 reply; 7+ messages in thread From: Shirish Pargaonkar @ 2009-03-27 15:15 UTC (permalink / raw) To: Steve French, Jeff Layton, Suresh Jayaraman Cc: linux-cifs-client@lists.samba.org, linux-fsdevel [-- Attachment #1: Type: text/plain, Size: 3392 bytes --] On Thu, Mar 19, 2009 at 3:36 PM, Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote: > On Fri, Feb 27, 2009 at 2:34 PM, Shirish Pargaonkar > <shirishpargaonkar@gmail.com> wrote: >> While there are pending issues related to broken Samba server posix >> code/behaviour, >> I thought I will run by you the patch for lookup intent nonetheless. >> >> It worked on 2.6.29-rc3 kernel with some of the testing I have done, I >> just downloaded >> the git sources from Steve's tree and ported the patch, that is what this is. >> I am building those sources and should boot with that kernel and test >> the patch and post >> the results here. >> >> Regards, >> >> Shirish >> > > Here is the second version of the lookup intent patch that takes into > consideration the samba posix open bug. > > I ran modified connectathon test1 to create 5000 files. > I am not sure whether existing code when does not send SET_PATH_INFO > with info level > SMB_SET_FILE_END_OF_FILE_INFO2 for O_TRUNC flag to a file open/creat > is a bug or not, > looking into it. > > > With the cthon test1 test, the files are created using > creat(<file_name>, 0666) meaning flags of > O_WRONLY, O_CREAT, O_TRUNC. > > With the current code, the O_TRUNC is not honoured i.e. I do not > see SMB_SET_FILE_END_OF_FILE_INFO2 info level going over the wire, > hence the faster creation of the files. > > I changed the code in test1.c by changing the call > creat(name, CHMOD_RW) > to > open(name, O_WRONLY | O_CREAT, CHMOD_RW) > and I can see open with lookup intent is definitely faster (28%). > > > > ---------------------- lookup intent and cifs_posix_open with creat > -------------------- > > + date > Thu Mar 19 11:26:03 CDT 2009 > + ./test1 -f > ./test1: File and directory creation test > test1 files 2 created 5000 files 1 directories 1 levels deep > ./test1 ok. > + date > Thu Mar 19 11:26:29 CDT 2009 > > > SMB_POSIX_OPEN > SMB_SET_FILE_END_OF_FILE_INFO2 > SMB_SET_FILE_UNIX_BASIC > > > ---------------------- no lookup intent and cifs_posix_open with creat > ------------------ > > + date > Thu Mar 19 11:28:43 CDT 2009 > + ./test1 -f > > ./test1: File and directory creation test > test1 files 2 created 5000 files 1 directories 1 levels deep > ./test1 ok. > + date > Thu Mar 19 11:29:01 CDT 2009 > > SMB_POSIX_OPEN > > ----------------------------------------------------------------------------- > > > > > ---------------------- lookup intent and cifs_posix_open with open > -------------------- > > + date > Thu Mar 19 15:19:32 CDT 2009 > + ./test1 -f > ./test1: File and directory creation test > test1 files 2 created 5000 files 0 directories 1 levels deep > ./test1 ok. > + date > Thu Mar 19 15:19:45 CDT 2009 > > > ---------------------- no lookup intent and cifs_posix_open with open > ------------------ > > + date > Thu Mar 19 15:21:07 CDT 2009 > + ./test1 -f > ./test1: File and directory creation test > test1 files 2 created 5000 files 0 directories 1 levels deep > ./test1 ok. > + date > Thu Mar 19 15:21:25 CDT 2009 > > > ----------------------------------------------------------------------------- > Here is the third iteration of the lookup intent patch for cifs, this time mode is set with taking umaks into consideration. Regards, Shirish [-- Attachment #2: li.3.patch --] [-- Type: application/octet-stream, Size: 7967 bytes --] 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); + 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); 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) { ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: lookup intent patch 2009-03-27 15:15 ` lookup intent patch Shirish Pargaonkar @ 2009-03-27 18:13 ` Jeff Layton 2009-03-30 15:57 ` Shirish Pargaonkar 0 siblings, 1 reply; 7+ messages in thread From: Jeff Layton @ 2009-03-27 18:13 UTC (permalink / raw) To: Shirish Pargaonkar Cc: Steve French, Suresh Jayaraman, linux-cifs-client@lists.samba.org, linux-fsdevel On Fri, 27 Mar 2009 10:15:21 -0500 Shirish Pargaonkar <shirishpargaonkar@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@redhat.com> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: lookup intent patch 2009-03-27 18:13 ` Jeff Layton @ 2009-03-30 15:57 ` Shirish Pargaonkar 2009-03-30 17:45 ` [linux-cifs-client] " Jeff Layton 0 siblings, 1 reply; 7+ messages in thread From: Shirish Pargaonkar @ 2009-03-30 15:57 UTC (permalink / raw) To: Jeff Layton Cc: Steve French, Suresh Jayaraman, linux-cifs-client@lists.samba.org, linux-fsdevel On Fri, Mar 27, 2009 at 1:13 PM, Jeff Layton <jlayton@redhat.com> wrote: > On Fri, 27 Mar 2009 10:15:21 -0500 > Shirish Pargaonkar <shirishpargaonkar@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@redhat.com> > Jeff, Thanks. Looking into it. I am trying to figure out the need/necessity for cifs_lookup to call lookup_instanitate_flip. lookup_instantiate_filp does call dentry_open and if cifs_lookup does not call lookup_instantiate_flip, nameidata_to_filp will call dentry_open. So I am not sure what we loose if dentry_open does not get called between lookup_hash and nameidata_to_flip because of an error between those two calls, specifically how will the cause of open file getting closed on the server will be served if there was an in-betwen error by calling lookup_instantiate_filp. Regards, Shirish -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [linux-cifs-client] Re: lookup intent patch 2009-03-30 15:57 ` Shirish Pargaonkar @ 2009-03-30 17:45 ` Jeff Layton 2009-03-30 20:07 ` Shirish Pargaonkar 0 siblings, 1 reply; 7+ messages in thread From: Jeff Layton @ 2009-03-30 17:45 UTC (permalink / raw) To: Shirish Pargaonkar Cc: linux-fsdevel, Steve French, linux-cifs-client@lists.samba.org On Mon, 30 Mar 2009 10:57:32 -0500 Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote: > > Jeff, > > Thanks. Looking into it. I am trying to figure out the need/necessity > for cifs_lookup to call lookup_instanitate_flip. > lookup_instantiate_filp does call dentry_open and if cifs_lookup does > not call lookup_instantiate_flip, > nameidata_to_filp will call dentry_open. > So I am not sure what we loose if dentry_open does not get called > between lookup_hash and nameidata_to_flip > because of an error between those two calls, specifically how will the > cause of open file getting closed on the > server will be served if there was an in-betwen error by calling > lookup_instantiate_filp. > I'm not certain since I haven't tested your patch, but you may end up with an inode refcount leak (aka Busy inodes after umount...). You're doing an open on the file in the lookup and I think that increases the refcount of the inode (i_count). Eventually, that inode gets "put" when you close the file. In the error situation described above though, that put will never occur. As far as the VFS is concerned, the file was never actually opened, so it doesn't need to issue a fput(). Properly cleaning up the references is the main reason to make sure that you pass the filp back to the caller here. Closing the open file on the server is also a nice side benefit since that could block the granting of oplocks and such. -- Jeff Layton <jlayton@redhat.com> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [linux-cifs-client] Re: lookup intent patch 2009-03-30 17:45 ` [linux-cifs-client] " Jeff Layton @ 2009-03-30 20:07 ` Shirish Pargaonkar 2009-03-31 0:29 ` Jeff Layton 0 siblings, 1 reply; 7+ messages in thread From: Shirish Pargaonkar @ 2009-03-30 20:07 UTC (permalink / raw) To: Jeff Layton Cc: linux-fsdevel, Steve French, linux-cifs-client@lists.samba.org On Mon, Mar 30, 2009 at 12:45 PM, Jeff Layton <jlayton@redhat.com> wrote: > On Mon, 30 Mar 2009 10:57:32 -0500 > Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote: >> >> Jeff, >> >> Thanks. Looking into it. I am trying to figure out the need/necessity >> for cifs_lookup to call lookup_instanitate_flip. >> lookup_instantiate_filp does call dentry_open and if cifs_lookup does >> not call lookup_instantiate_flip, >> nameidata_to_filp will call dentry_open. >> So I am not sure what we loose if dentry_open does not get called >> between lookup_hash and nameidata_to_flip >> because of an error between those two calls, specifically how will the >> cause of open file getting closed on the >> server will be served if there was an in-betwen error by calling >> lookup_instantiate_filp. >> > > I'm not certain since I haven't tested your patch, but you may end up > with an inode refcount leak (aka Busy inodes after umount...). You're > doing an open on the file in the lookup and I think that increases the > refcount of the inode (i_count). Eventually, that inode gets "put" when > you close the file. In the error situation described above though, that > put will never occur. As far as the VFS is concerned, the file was > never actually opened, so it doesn't need to issue a fput(). We would still be in do_flip_open and so if there is an error, while exiting release_open_intent would get called which would so the cleanup i.e. call fput(). Let me introduce an error in between to verify whether the data structures are cleaned up, such as i_count of an inode. > > Properly cleaning up the references is the main reason to make sure > that you pass the filp back to the caller here. Closing the open file > on the server is also a nice side benefit since that could block the > granting of oplocks and such. > I think caller is oblivious to the speed-up mechanism that cifs is attempting by taking advantage of lookup intents to reduce network traffic. > -- > Jeff Layton <jlayton@redhat.com> > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [linux-cifs-client] Re: lookup intent patch 2009-03-30 20:07 ` Shirish Pargaonkar @ 2009-03-31 0:29 ` Jeff Layton 2009-03-31 9:25 ` Shirish Pargaonkar 0 siblings, 1 reply; 7+ messages in thread From: Jeff Layton @ 2009-03-31 0:29 UTC (permalink / raw) To: Shirish Pargaonkar Cc: linux-fsdevel, Steve French, linux-cifs-client@lists.samba.org On Mon, 30 Mar 2009 15:07:23 -0500 Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote: > On Mon, Mar 30, 2009 at 12:45 PM, Jeff Layton <jlayton@redhat.com> wrote: > > On Mon, 30 Mar 2009 10:57:32 -0500 > > Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote: > >> > >> Jeff, > >> > >> Thanks. Looking into it. I am trying to figure out the need/necessity > >> for cifs_lookup to call lookup_instanitate_flip. > >> lookup_instantiate_filp does call dentry_open and if cifs_lookup does > >> not call lookup_instantiate_flip, > >> nameidata_to_filp will call dentry_open. > >> So I am not sure what we loose if dentry_open does not get called > >> between lookup_hash and nameidata_to_flip > >> because of an error between those two calls, specifically how will the > >> cause of open file getting closed on the > >> server will be served if there was an in-betwen error by calling > >> lookup_instantiate_filp. > >> > > > > I'm not certain since I haven't tested your patch, but you may end up > > with an inode refcount leak (aka Busy inodes after umount...). You're > > doing an open on the file in the lookup and I think that increases the > > refcount of the inode (i_count). Eventually, that inode gets "put" when > > you close the file. In the error situation described above though, that > > put will never occur. As far as the VFS is concerned, the file was > > never actually opened, so it doesn't need to issue a fput(). > > We would still be in do_flip_open and so if there is an error, while exiting > release_open_intent would get called which would so the cleanup i.e. > call fput(). release_open_intent only calls fput if there is a filp set in the open_intent info. With your patch, you won't have one. Well...you'll have an empty filp, but I'm not sure it'll have all of the fields that are needed to actually make release_open_intent call fput(). In particular, I don't think f_path.dentry will be set. > Let me introduce an error in between to verify whether the data structures > are cleaned up, such as i_count of an inode. > > > > > Properly cleaning up the references is the main reason to make sure > > that you pass the filp back to the caller here. Closing the open file > > on the server is also a nice side benefit since that could block the > > granting of oplocks and such. > > > > I think caller is oblivious to the speed-up mechanism that cifs is attempting > by taking advantage of lookup intents to reduce network traffic. > Right -- and that's a problem since it won't clean up the references unless it knows this. -- Jeff Layton <jlayton@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [linux-cifs-client] Re: lookup intent patch 2009-03-31 0:29 ` Jeff Layton @ 2009-03-31 9:25 ` Shirish Pargaonkar 0 siblings, 0 replies; 7+ messages in thread From: Shirish Pargaonkar @ 2009-03-31 9:25 UTC (permalink / raw) To: Jeff Layton Cc: linux-fsdevel, Steve French, linux-cifs-client@lists.samba.org On Mon, Mar 30, 2009 at 7:29 PM, Jeff Layton <jlayton@redhat.com> wrote: > On Mon, 30 Mar 2009 15:07:23 -0500 > Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote: > >> On Mon, Mar 30, 2009 at 12:45 PM, Jeff Layton <jlayton@redhat.com> wrote: >> > On Mon, 30 Mar 2009 10:57:32 -0500 >> > Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote: >> >> >> >> Jeff, >> >> >> >> Thanks. Looking into it. I am trying to figure out the need/necessity >> >> for cifs_lookup to call lookup_instanitate_flip. >> >> lookup_instantiate_filp does call dentry_open and if cifs_lookup does >> >> not call lookup_instantiate_flip, >> >> nameidata_to_filp will call dentry_open. >> >> So I am not sure what we loose if dentry_open does not get called >> >> between lookup_hash and nameidata_to_flip >> >> because of an error between those two calls, specifically how will the >> >> cause of open file getting closed on the >> >> server will be served if there was an in-betwen error by calling >> >> lookup_instantiate_filp. >> >> >> > >> > I'm not certain since I haven't tested your patch, but you may end up >> > with an inode refcount leak (aka Busy inodes after umount...). You're >> > doing an open on the file in the lookup and I think that increases the >> > refcount of the inode (i_count). Eventually, that inode gets "put" when >> > you close the file. In the error situation described above though, that >> > put will never occur. As far as the VFS is concerned, the file was >> > never actually opened, so it doesn't need to issue a fput(). >> >> We would still be in do_flip_open and so if there is an error, while exiting >> release_open_intent would get called which would so the cleanup i.e. >> call fput(). > > release_open_intent only calls fput if there is a filp set in the > open_intent info. With your patch, you won't have one. > > Well...you'll have an empty filp, but I'm not sure it'll have all of > the fields that are needed to actually make release_open_intent call > fput(). In particular, I don't think f_path.dentry will be set. > >> Let me introduce an error in between to verify whether the data structures >> are cleaned up, such as i_count of an inode. >> >> > >> > Properly cleaning up the references is the main reason to make sure >> > that you pass the filp back to the caller here. Closing the open file >> > on the server is also a nice side benefit since that could block the >> > granting of oplocks and such. >> > >> >> I think caller is oblivious to the speed-up mechanism that cifs is attempting >> by taking advantage of lookup intents to reduce network traffic. >> > > Right -- and that's a problem since it won't clean up the references > unless it knows this. > > -- > Jeff Layton <jlayton@redhat.com> > OK, let me make those changes and I will re-post the patch on a different thread with appropriate subject line. Regards, Shirish ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2009-03-31 9:25 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <4a4634330902271134h3f334febke42b67ceab7e16eb@mail.gmail.com>
[not found] ` <4a4634330903191336n54758971r2ff809ba31a80791@mail.gmail.com>
2009-03-27 15:15 ` lookup intent patch Shirish Pargaonkar
2009-03-27 18:13 ` Jeff Layton
2009-03-30 15:57 ` Shirish Pargaonkar
2009-03-30 17:45 ` [linux-cifs-client] " Jeff Layton
2009-03-30 20:07 ` Shirish Pargaonkar
2009-03-31 0:29 ` Jeff Layton
2009-03-31 9:25 ` Shirish Pargaonkar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).