On Fri, Mar 27, 2009 at 1:13 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> On Fri, 27 Mar 2009 10:15:21 -0500
> Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> 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@xxxxxxxxxx>
>
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
