| Message ID | 20210308155229.GD7284@fieldses.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] Revert "nfsd4: remove check_conflicting_opens warning" | expand |
> On Mar 8, 2021, at 10:52 AM, J. Bruce Fields <bfields@fieldses.org> wrote: > > From: "J. Bruce Fields" <bfields@redhat.com> > > This reverts commit 94415b06eb8aed13481646026dc995f04a3a534a. > > That commit claimed to allow a client to get a read delegation when it > was the only writer. Actually it allowed a client to get a read > delegation when *any* client has a write open! > > The main problem is that it's depending on nfs4_clnt_odstate structures > that are actually only maintained for pnfs exports. > > This causes clients to miss writes performed by other clients, even when > there have been intervening closes and opens, violating close-to-open > cache consistency. > > We can do this a different way, but first we should just revert this. > > I've added pynfs 4.1 test DELEG19 to test for this, as I should have > done originally! Excellent, Bruce. I'll get these into for-rc right away so it can get some testing with the 0-day infrastructure. > Cc: stable@vger.kernel.org > Reported-by: Timo Rothenpieler <timo@rothenpieler.org> > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > --- > fs/locks.c | 3 --- > fs/nfsd/nfs4state.c | 54 ++++++++++++--------------------------------- > 2 files changed, 14 insertions(+), 43 deletions(-) > > diff --git a/fs/locks.c b/fs/locks.c > index 99ca97e81b7a..6125d2de39b8 100644 > --- a/fs/locks.c > +++ b/fs/locks.c > @@ -1808,9 +1808,6 @@ check_conflicting_open(struct file *filp, const long arg, int flags) > > if (flags & FL_LAYOUT) > return 0; > - if (flags & FL_DELEG) > - /* We leave these checks to the caller. */ > - return 0; > > if (arg == F_RDLCK) > return inode_is_open_for_write(inode) ? -EAGAIN : 0; > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 408e2c4db926..27983a50435e 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -4940,32 +4940,6 @@ static struct file_lock *nfs4_alloc_init_lease(struct nfs4_delegation *dp, > return fl; > } > > -static int nfsd4_check_conflicting_opens(struct nfs4_client *clp, > - struct nfs4_file *fp) > -{ > - struct nfs4_clnt_odstate *co; > - struct file *f = fp->fi_deleg_file->nf_file; > - struct inode *ino = locks_inode(f); > - int writes = atomic_read(&ino->i_writecount); > - > - if (fp->fi_fds[O_WRONLY]) > - writes--; > - if (fp->fi_fds[O_RDWR]) > - writes--; > - WARN_ON_ONCE(writes < 0); > - if (writes > 0) > - return -EAGAIN; > - spin_lock(&fp->fi_lock); > - list_for_each_entry(co, &fp->fi_clnt_odstate, co_perfile) { > - if (co->co_client != clp) { > - spin_unlock(&fp->fi_lock); > - return -EAGAIN; > - } > - } > - spin_unlock(&fp->fi_lock); > - return 0; > -} > - > static struct nfs4_delegation * > nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh, > struct nfs4_file *fp, struct nfs4_clnt_odstate *odstate) > @@ -4985,12 +4959,9 @@ nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh, > > nf = find_readable_file(fp); > if (!nf) { > - /* > - * We probably could attempt another open and get a read > - * delegation, but for now, don't bother until the > - * client actually sends us one. > - */ > - return ERR_PTR(-EAGAIN); > + /* We should always have a readable file here */ > + WARN_ON_ONCE(1); > + return ERR_PTR(-EBADF); > } > spin_lock(&state_lock); > spin_lock(&fp->fi_lock); > @@ -5020,19 +4991,11 @@ nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh, > if (!fl) > goto out_clnt_odstate; > > - status = nfsd4_check_conflicting_opens(clp, fp); > - if (status) { > - locks_free_lock(fl); > - goto out_clnt_odstate; > - } > status = vfs_setlease(fp->fi_deleg_file->nf_file, fl->fl_type, &fl, NULL); > if (fl) > locks_free_lock(fl); > if (status) > goto out_clnt_odstate; > - status = nfsd4_check_conflicting_opens(clp, fp); > - if (status) > - goto out_clnt_odstate; > > spin_lock(&state_lock); > spin_lock(&fp->fi_lock); > @@ -5114,6 +5077,17 @@ nfs4_open_delegation(struct svc_fh *fh, struct nfsd4_open *open, > goto out_no_deleg; > if (!cb_up || !(oo->oo_flags & NFS4_OO_CONFIRMED)) > goto out_no_deleg; > + /* > + * Also, if the file was opened for write or > + * create, there's a good chance the client's > + * about to write to it, resulting in an > + * immediate recall (since we don't support > + * write delegations): > + */ > + if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) > + goto out_no_deleg; > + if (open->op_create == NFS4_OPEN_CREATE) > + goto out_no_deleg; > break; > default: > goto out_no_deleg; > -- > 2.29.2 > -- Chuck Lever
diff --git a/fs/locks.c b/fs/locks.c index 99ca97e81b7a..6125d2de39b8 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -1808,9 +1808,6 @@ check_conflicting_open(struct file *filp, const long arg, int flags) if (flags & FL_LAYOUT) return 0; - if (flags & FL_DELEG) - /* We leave these checks to the caller. */ - return 0; if (arg == F_RDLCK) return inode_is_open_for_write(inode) ? -EAGAIN : 0; diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 408e2c4db926..27983a50435e 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -4940,32 +4940,6 @@ static struct file_lock *nfs4_alloc_init_lease(struct nfs4_delegation *dp, return fl; } -static int nfsd4_check_conflicting_opens(struct nfs4_client *clp, - struct nfs4_file *fp) -{ - struct nfs4_clnt_odstate *co; - struct file *f = fp->fi_deleg_file->nf_file; - struct inode *ino = locks_inode(f); - int writes = atomic_read(&ino->i_writecount); - - if (fp->fi_fds[O_WRONLY]) - writes--; - if (fp->fi_fds[O_RDWR]) - writes--; - WARN_ON_ONCE(writes < 0); - if (writes > 0) - return -EAGAIN; - spin_lock(&fp->fi_lock); - list_for_each_entry(co, &fp->fi_clnt_odstate, co_perfile) { - if (co->co_client != clp) { - spin_unlock(&fp->fi_lock); - return -EAGAIN; - } - } - spin_unlock(&fp->fi_lock); - return 0; -} - static struct nfs4_delegation * nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh, struct nfs4_file *fp, struct nfs4_clnt_odstate *odstate) @@ -4985,12 +4959,9 @@ nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh, nf = find_readable_file(fp); if (!nf) { - /* - * We probably could attempt another open and get a read - * delegation, but for now, don't bother until the - * client actually sends us one. - */ - return ERR_PTR(-EAGAIN); + /* We should always have a readable file here */ + WARN_ON_ONCE(1); + return ERR_PTR(-EBADF); } spin_lock(&state_lock); spin_lock(&fp->fi_lock); @@ -5020,19 +4991,11 @@ nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh, if (!fl) goto out_clnt_odstate; - status = nfsd4_check_conflicting_opens(clp, fp); - if (status) { - locks_free_lock(fl); - goto out_clnt_odstate; - } status = vfs_setlease(fp->fi_deleg_file->nf_file, fl->fl_type, &fl, NULL); if (fl) locks_free_lock(fl); if (status) goto out_clnt_odstate; - status = nfsd4_check_conflicting_opens(clp, fp); - if (status) - goto out_clnt_odstate; spin_lock(&state_lock); spin_lock(&fp->fi_lock); @@ -5114,6 +5077,17 @@ nfs4_open_delegation(struct svc_fh *fh, struct nfsd4_open *open, goto out_no_deleg; if (!cb_up || !(oo->oo_flags & NFS4_OO_CONFIRMED)) goto out_no_deleg; + /* + * Also, if the file was opened for write or + * create, there's a good chance the client's + * about to write to it, resulting in an + * immediate recall (since we don't support + * write delegations): + */ + if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) + goto out_no_deleg; + if (open->op_create == NFS4_OPEN_CREATE) + goto out_no_deleg; break; default: goto out_no_deleg;