On Tue, 4 Jan 2011 09:17:26 -0600
Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
> On Tue, Jan 4, 2011 at 7:16 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> > We call CIFSSMBUnixSetPathInfo in these functions, but we have a
> > filehandle since an open was just done. Switch these functions to
> > use CIFSSMBUnixSetFileInfo instead.
> >
> > In practice, these codepaths are only used if posix opens are broken.
> >
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > ---
> > fs/cifs/dir.c | 6 ++----
> > fs/cifs/file.c | 6 ++----
> > 2 files changed, 4 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> > index 521d841..d9b46bf 100644
> > --- a/fs/cifs/dir.c
> > +++ b/fs/cifs/dir.c
> > @@ -293,10 +293,8 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
> > args.uid = NO_CHANGE_64;
> > args.gid = NO_CHANGE_64;
> > }
> > - CIFSSMBUnixSetPathInfo(xid, tcon, full_path, &args,
> > - cifs_sb->local_nls,
> > - cifs_sb->mnt_cifs_flags &
> > - CIFS_MOUNT_MAP_SPECIAL_CHR);
> > + CIFSSMBUnixSetFileInfo(xid, tcon, &args, fileHandle,
> > + current->tgid);
> > } else {
> > /* BB implement mode setting via Windows security
> > descriptors e.g. */
> > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > index a7b8a37..4007e60 100644
> > --- a/fs/cifs/file.c
> > +++ b/fs/cifs/file.c
> > @@ -424,10 +424,8 @@ int cifs_open(struct inode *inode, struct file *file)
> > .mtime = NO_CHANGE_64,
> > .device = 0,
> > };
> > - CIFSSMBUnixSetPathInfo(xid, tcon, full_path, &args,
> > - cifs_sb->local_nls,
> > - cifs_sb->mnt_cifs_flags &
> > - CIFS_MOUNT_MAP_SPECIAL_CHR);
> > + CIFSSMBUnixSetFileInfo(xid, tcon, &args, netfid,
> > + pCifsFile->pid);
> > }
> >
> > out:
> > --
> > 1.7.3.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
>
> Would a filehandle always remain valid between open and set?
> Could there be a reconnect in between these calls thus making
> a filehandle invalid and failing a setfileinfo which otherwise would
> succeed with setpathinfo?
Seems unlikely, but I suppose it's possible.
Unfortunately, cifs is pretty racy in this regard. Suppose we do this:
if (open_file->invalidHandle) {
rc = cifs_reopen_file(open_file, false);
if (rc != 0)
break;
}
CIFSSMBUnixSetFileInfo(...);
...it's always possible that the handle will flip to being invalid
after you've done the check but before the call goes out on the wire.
It's also possible that the file was reclaimed with a different number
before you do this check. The main things that save us is that
reconnects are relatively uncommon, and they generally take a long time
so those sorts of races don't generally happen much.
What we really need is a better way to deal with this that doesn't rely
on these sorts of checks. I have a scheme in mind to do this, but it
requires overhauling quite a bit of the SendReceive code. I don't want
to embark on that work until some of my existing patches has been merged.
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
