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

[Jeff Layton]

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?

-- 
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/