Re: [PATCH] cifs: don't update uniqueid in cifs_fattr_to_inode

On Fri, 7 May 2010 09:32:57 -0500
Steve French <[email protected]> wrote:

> On Fri, May 7, 2010 at 9:03 AM, Jeff Layton <[email protected]> wrote:
> > We use this value to find an inode within the hash bucket, so we can't
> > change this without re-hashing the inode. For now, treat this value
> > as immutable.
> >
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> >  fs/cifs/inode.c |    1 -
> >  1 files changed, 0 insertions(+), 1 deletions(-)
> >
> > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> > index 35ec117..ee35c97 100644
> > --- a/fs/cifs/inode.c
> > +++ b/fs/cifs/inode.c
> > @@ -137,7 +137,6 @@ cifs_fattr_to_inode(struct inode *inode, struct cifs_fattr *fattr)
> >                inode->i_mode = fattr->cf_mode;
> >
> >        cifs_i->cifsAttrs = fattr->cf_cifsattrs;
> > -       cifs_i->uniqueid = fattr->cf_uniqueid;
> >
> >        if (fattr->cf_flags & CIFS_FATTR_NEED_REVAL)
> >                cifs_i->time = 0;
> 
> Seems like there is a case where this is worse (imagine renaming a
> file, creating a new
> file with the old path)
> - We have an inode with particular values (size, attributes etc.) that
> are newly updated
> - We leave the old inode number
> 
> So we have a new different file with the same name but the old inode
> number, with
> the new file's attributes which seems confusing
> to me.   Perhaps in this case it is safer to mark the old inode stale
> and unhash it
> (similar to your other patch)
> 
> 

Good point. Let me think on this a bit. We clearly have a bug here, but
you may be right that it's not terribly harmful. I think we have some
work to do in this area however.

-- 
Jeff Layton <[email protected]>