| Message ID | CANT5p=qXbQU4g4VX=W9mOQo1SjMxnFGfpkLOJWgCpicyDLvt-w@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cifs: wait for tcon resource_id before getting fscache super | expand |
Your patches all got mangled. Spaces converted to tabs. David
These are what I had downloaded and tentatively merged into cifs-2.6.git for-next On Fri, Dec 3, 2021 at 3:23 AM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > The logic for initializing tcon->resource_id is done inside > cifs_root_iget. fscache super cookie relies on this for aux > data. So we need to push the fscache initialization to this > later point during mount. > > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com> > --- > fs/cifs/connect.c | 6 ------ > fs/cifs/fscache.c | 2 +- > fs/cifs/inode.c | 7 +++++++ > 3 files changed, 8 insertions(+), 7 deletions(-) > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > index 6b705026da1a3..eee994b0925ff 100644 > --- a/fs/cifs/connect.c > +++ b/fs/cifs/connect.c > @@ -3046,12 +3046,6 @@ static int mount_get_conns(struct mount_ctx *mnt_ctx) > cifs_dbg(VFS, "read only mount of RW share\n"); > /* no need to log a RW mount of a typical RW share */ > } > - /* > - * The cookie is initialized from volume info returned above. > - * Inside cifs_fscache_get_super_cookie it checks > - * that we do not get super cookie twice. > - */ > - cifs_fscache_get_super_cookie(tcon); > } > > /* > diff --git a/fs/cifs/fscache.c b/fs/cifs/fscache.c > index 7e409a38a2d7c..f4da693760c11 100644 > --- a/fs/cifs/fscache.c > +++ b/fs/cifs/fscache.c > @@ -92,7 +92,7 @@ void cifs_fscache_get_super_cookie(struct cifs_tcon *tcon) > * In the future, as we integrate with newer fscache features, > * we may want to instead add a check if cookie has changed > */ > - if (tcon->fscache == NULL) > + if (tcon->fscache) > return; > > sharename = extract_sharename(tcon->treeName); > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c > index 82848412ad852..96d083db17372 100644 > --- a/fs/cifs/inode.c > +++ b/fs/cifs/inode.c > @@ -1376,6 +1376,13 @@ struct inode *cifs_root_iget(struct super_block *sb) > inode = ERR_PTR(rc); > } > > + /* > + * The cookie is initialized from volume info returned above. > + * Inside cifs_fscache_get_super_cookie it checks > + * that we do not get super cookie twice. > + */ > + cifs_fscache_get_super_cookie(tcon); > + > out: > kfree(path); > free_xid(xid);
On Fri, 2021-12-03 at 14:52 +0530, Shyam Prasad N wrote: > The logic for initializing tcon->resource_id is done inside > cifs_root_iget. fscache super cookie relies on this for aux > data. So we need to push the fscache initialization to this > later point during mount. > > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com> > --- > fs/cifs/connect.c | 6 ------ > fs/cifs/fscache.c | 2 +- > fs/cifs/inode.c | 7 +++++++ > 3 files changed, 8 insertions(+), 7 deletions(-) > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > index 6b705026da1a3..eee994b0925ff 100644 > --- a/fs/cifs/connect.c > +++ b/fs/cifs/connect.c > @@ -3046,12 +3046,6 @@ static int mount_get_conns(struct mount_ctx *mnt_ctx) > cifs_dbg(VFS, "read only mount of RW share\n"); > /* no need to log a RW mount of a typical RW share */ > } > - /* > - * The cookie is initialized from volume info returned above. > - * Inside cifs_fscache_get_super_cookie it checks > - * that we do not get super cookie twice. > - */ > - cifs_fscache_get_super_cookie(tcon); > } > > /* > diff --git a/fs/cifs/fscache.c b/fs/cifs/fscache.c > index 7e409a38a2d7c..f4da693760c11 100644 > --- a/fs/cifs/fscache.c > +++ b/fs/cifs/fscache.c > @@ -92,7 +92,7 @@ void cifs_fscache_get_super_cookie(struct cifs_tcon *tcon) > * In the future, as we integrate with newer fscache features, > * we may want to instead add a check if cookie has changed > */ > - if (tcon->fscache == NULL) > + if (tcon->fscache) > return; > Ouch! Does the above mean that fscache on cifs is just plain broken at the moment? If this is the routine that sets the tcon cookie, then it looks like it just never gets set? > sharename = extract_sharename(tcon->treeName); > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c > index 82848412ad852..96d083db17372 100644 > --- a/fs/cifs/inode.c > +++ b/fs/cifs/inode.c > @@ -1376,6 +1376,13 @@ struct inode *cifs_root_iget(struct super_block *sb) > inode = ERR_PTR(rc); > } > > + /* > + * The cookie is initialized from volume info returned above. > + * Inside cifs_fscache_get_super_cookie it checks > + * that we do not get super cookie twice. > + */ > + cifs_fscache_get_super_cookie(tcon); > + > out: > kfree(path); > free_xid(xid); >
On December 3, 2021 1:21:15 PM GMT-03:00, Jeff Layton <jlayton@redhat.com> wrote: >On Fri, 2021-12-03 at 14:52 +0530, Shyam Prasad N wrote: >> The logic for initializing tcon->resource_id is done inside >> cifs_root_iget. fscache super cookie relies on this for aux >> data. So we need to push the fscache initialization to this >> later point during mount. >> >> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com> >> --- >> fs/cifs/connect.c | 6 ------ >> fs/cifs/fscache.c | 2 +- >> fs/cifs/inode.c | 7 +++++++ >> 3 files changed, 8 insertions(+), 7 deletions(-) >> >> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c >> index 6b705026da1a3..eee994b0925ff 100644 >> --- a/fs/cifs/connect.c >> +++ b/fs/cifs/connect.c >> @@ -3046,12 +3046,6 @@ static int mount_get_conns(struct mount_ctx *mnt_ctx) >> cifs_dbg(VFS, "read only mount of RW share\n"); >> /* no need to log a RW mount of a typical RW share */ >> } >> - /* >> - * The cookie is initialized from volume info returned above. >> - * Inside cifs_fscache_get_super_cookie it checks >> - * that we do not get super cookie twice. >> - */ >> - cifs_fscache_get_super_cookie(tcon); >> } >> >> /* >> diff --git a/fs/cifs/fscache.c b/fs/cifs/fscache.c >> index 7e409a38a2d7c..f4da693760c11 100644 >> --- a/fs/cifs/fscache.c >> +++ b/fs/cifs/fscache.c >> @@ -92,7 +92,7 @@ void cifs_fscache_get_super_cookie(struct cifs_tcon *tcon) >> * In the future, as we integrate with newer fscache features, >> * we may want to instead add a check if cookie has changed >> */ >> - if (tcon->fscache == NULL) >> + if (tcon->fscache) >> return; >> > >Ouch! Does the above mean that fscache on cifs is just plain broken at >the moment? If this is the routine that sets the tcon cookie, then it >looks like it just never gets set? Dont much know about fscache, but remember that multiple mounts can share a single tcon (if not using nosharesock). So, if we find an existing tcon and we have a cookie for it already, the check makes sense. > >> sharename = extract_sharename(tcon->treeName); >> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c >> index 82848412ad852..96d083db17372 100644 >> --- a/fs/cifs/inode.c >> +++ b/fs/cifs/inode.c >> @@ -1376,6 +1376,13 @@ struct inode *cifs_root_iget(struct super_block *sb) >> inode = ERR_PTR(rc); >> } >> >> + /* >> + * The cookie is initialized from volume info returned above. >> + * Inside cifs_fscache_get_super_cookie it checks >> + * that we do not get super cookie twice. >> + */ >> + cifs_fscache_get_super_cookie(tcon); >> + >> out: >> kfree(path); >> free_xid(xid); >> >
Shyam Prasad N <nspmangalore@gmail.com> wrote: > @@ -1376,6 +1376,13 @@ struct inode *cifs_root_iget(struct super_block *sb) > inode = ERR_PTR(rc); > } > > + /* > + * The cookie is initialized from volume info returned above. > + * Inside cifs_fscache_get_super_cookie it checks > + * that we do not get super cookie twice. > + */ > + cifs_fscache_get_super_cookie(tcon); Ummm... Does this handle the errors correctly? What happens if rc != 0 at this point and the inode has been marked failed? It looks like it will abandon creation of the superblock without cleaning up the super cookie. Maybe - or maybe it can't happen because of the: iget_no_retry: if (!inode) { inode = ERR_PTR(rc); goto out; } check - but then why is rc being checked? > + > out: > kfree(path); > free_xid(xid); David
On Mon, Dec 6, 2021 at 7:22 PM David Howells <dhowells@redhat.com> wrote: > > Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > @@ -1376,6 +1376,13 @@ struct inode *cifs_root_iget(struct super_block *sb) > > inode = ERR_PTR(rc); > > } > > > > + /* > > + * The cookie is initialized from volume info returned above. > > + * Inside cifs_fscache_get_super_cookie it checks > > + * that we do not get super cookie twice. > > + */ > > + cifs_fscache_get_super_cookie(tcon); > > Ummm... Does this handle the errors correctly? What happens if rc != 0 at > this point and the inode has been marked failed? It looks like it will > abandon creation of the superblock without cleaning up the super cookie. > Maybe - or maybe it can't happen because of the: > > iget_no_retry: > if (!inode) { > inode = ERR_PTR(rc); > goto out; > } > > check - but then why is rc being checked? > > > + > > out: > > kfree(path); > > free_xid(xid); > > David > Thanks David. I think that there still needs to be more error handling here. I'll check on this and send out another patch.
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index 6b705026da1a3..eee994b0925ff 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -3046,12 +3046,6 @@ static int mount_get_conns(struct mount_ctx *mnt_ctx) cifs_dbg(VFS, "read only mount of RW share\n"); /* no need to log a RW mount of a typical RW share */ } - /* - * The cookie is initialized from volume info returned above. - * Inside cifs_fscache_get_super_cookie it checks - * that we do not get super cookie twice. - */ - cifs_fscache_get_super_cookie(tcon); } /* diff --git a/fs/cifs/fscache.c b/fs/cifs/fscache.c index 7e409a38a2d7c..f4da693760c11 100644 --- a/fs/cifs/fscache.c +++ b/fs/cifs/fscache.c @@ -92,7 +92,7 @@ void cifs_fscache_get_super_cookie(struct cifs_tcon *tcon) * In the future, as we integrate with newer fscache features, * we may want to instead add a check if cookie has changed */ - if (tcon->fscache == NULL) + if (tcon->fscache) return; sharename = extract_sharename(tcon->treeName); diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c index 82848412ad852..96d083db17372 100644 --- a/fs/cifs/inode.c +++ b/fs/cifs/inode.c @@ -1376,6 +1376,13 @@ struct inode *cifs_root_iget(struct super_block *sb) inode = ERR_PTR(rc); } + /* + * The cookie is initialized from volume info returned above. + * Inside cifs_fscache_get_super_cookie it checks + * that we do not get super cookie twice. + */ + cifs_fscache_get_super_cookie(tcon); + out: kfree(path); free_xid(xid);
The logic for initializing tcon->resource_id is done inside cifs_root_iget. fscache super cookie relies on this for aux data. So we need to push the fscache initialization to this later point during mount. Signed-off-by: Shyam Prasad N <sprasad@microsoft.com> --- fs/cifs/connect.c | 6 ------ fs/cifs/fscache.c | 2 +- fs/cifs/inode.c | 7 +++++++ 3 files changed, 8 insertions(+), 7 deletions(-)