Re: [PATCH v3 6/7] NFSv4: Add O_DENY* open flags support

[Jeff Layton]

On Thu, 4 Apr 2013 14:30:12 +0400
Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:

> 2013/3/12 Jeff Layton <jlayton@xxxxxxxxxx>:
> > On Mon, 11 Mar 2013 14:54:34 -0400
> > Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> >
> >> On Thu, 28 Feb 2013 19:25:32 +0400
> >> Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:
> >>
> >> > by passing these flags to NFSv4 open request.
> >> >
> >> > Signed-off-by: Pavel Shilovsky <piastry@xxxxxxxxxxx>
> >> > ---
> >> >  fs/nfs/nfs4xdr.c | 24 ++++++++++++++++++++----
> >> >  1 file changed, 20 insertions(+), 4 deletions(-)
> >> >
> >> > diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> >> > index 26b1439..58ddc74 100644
> >> > --- a/fs/nfs/nfs4xdr.c
> >> > +++ b/fs/nfs/nfs4xdr.c
> >> > @@ -1325,7 +1325,8 @@ static void encode_lookup(struct xdr_stream *xdr, const struct qstr *name, struc
> >> >     encode_string(xdr, name->len, name->name);
> >> >  }
> >> >
> >> > -static void encode_share_access(struct xdr_stream *xdr, fmode_t fmode)
> >> > +static void encode_share_access(struct xdr_stream *xdr, fmode_t fmode,
> >> > +                           int open_flags)
> >> >  {
> >> >     __be32 *p;
> >> >
> >> > @@ -1343,7 +1344,22 @@ static void encode_share_access(struct xdr_stream *xdr, fmode_t fmode)
> >> >     default:
> >> >             *p++ = cpu_to_be32(0);
> >> >     }
> >> > -   *p = cpu_to_be32(0);            /* for linux, share_deny = 0 always */
> >> > +   if (open_flags & O_DENYMAND) {
> >>
> >>
> >> As Bruce mentioned, I think a mount option to enable this on a per-fs
> >> basis would be a better approach than this new O_DENYMAND flag.
> >>
> >>
> >> > +           switch (open_flags & (O_DENYREAD|O_DENYWRITE)) {
> >> > +           case O_DENYREAD:
> >> > +                   *p = cpu_to_be32(NFS4_SHARE_DENY_READ);
> >> > +                   break;
> >> > +           case O_DENYWRITE:
> >> > +                   *p = cpu_to_be32(NFS4_SHARE_DENY_WRITE);
> >> > +                   break;
> >> > +           case O_DENYREAD|O_DENYWRITE:
> >> > +                   *p = cpu_to_be32(NFS4_SHARE_DENY_BOTH);
> >> > +                   break;
> >> > +           default:
> >> > +                   *p = cpu_to_be32(0);
> >> > +           }
> >> > +   } else
> >> > +           *p = cpu_to_be32(0);
> >> >  }
> >> >
> >> >  static inline void encode_openhdr(struct xdr_stream *xdr, const struct nfs_openargs *arg)
> >> > @@ -1354,7 +1370,7 @@ static inline void encode_openhdr(struct xdr_stream *xdr, const struct nfs_opena
> >> >   * owner 4 = 32
> >> >   */
> >> >     encode_nfs4_seqid(xdr, arg->seqid);
> >> > -   encode_share_access(xdr, arg->fmode);
> >> > +   encode_share_access(xdr, arg->fmode, arg->open_flags);
> >> >     p = reserve_space(xdr, 36);
> >> >     p = xdr_encode_hyper(p, arg->clientid);
> >> >     *p++ = cpu_to_be32(24);
> >> > @@ -1491,7 +1507,7 @@ static void encode_open_downgrade(struct xdr_stream *xdr, const struct nfs_close
> >> >     encode_op_hdr(xdr, OP_OPEN_DOWNGRADE, decode_open_downgrade_maxsz, hdr);
> >> >     encode_nfs4_stateid(xdr, arg->stateid);
> >> >     encode_nfs4_seqid(xdr, arg->seqid);
> >> > -   encode_share_access(xdr, arg->fmode);
> >> > +   encode_share_access(xdr, arg->fmode, 0);
> >> >  }
> >> >
> >> >  static void
> >>
> >>
> >> Other than that, this seems reasonable.
> >>
> >> Acked-by: Jeff Layton <jlayton@xxxxxxxxxx>
> >
> > Oh duh...
> >
> > Please ignore my comment on patch #7 to add a patch for the NFS client.
> > This one does that. That said, there may be a potential problem here
> > that you need to consider.
> >
> > In the case of a local filesystem you'll want to set deny locks using
> > deny_lock_file(). For a network filesystem like CIFS or NFS though,
> > the server will handle that atomically during the open. You need to
> > ensure that you don't go trying to set LOCK_MAND locks on the file once
> > that's done.
> >
> > Perhaps you can use a fstype flag to indicate that the filesystem
> > handles this during the open and doesn't need to try and set a flock
> > lock?
> 
> Also, we can simply mask off O_DENY* flags in open (and atomic_open)
> codepath of filesystems that support these flags:
> 
> ...
> do open request to the storage
> ...
> file->f_flags &= ~(O_DENYREAD | O_DENYWRITE | O_DENYDELETE);
> ...
> return to VFS
> ...
> 
> Thoughts?
> 

I'd probably still stick with a FS_* flag for this...

That sort of mechanism would work (for now) but sounds like the sort of
subtle behavior that's difficult for filesystem authors to get right.
It would also be subject to subtle breakage later.

Also, suppose there are changes in the future that require you to
determine this before calling into ->open? Then you'll have to go back
and somehow mark the fs anyway...

-- 
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/