Return-Path: linux-nfs-owner@vger.kernel.org Received: from mx1.redhat.com ([209.132.183.28]:27131 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1760548Ab3DDNCk (ORCPT ); Thu, 4 Apr 2013 09:02:40 -0400 Date: Thu, 4 Apr 2013 09:02:32 -0400 From: Jeff Layton To: Pavel Shilovsky Cc: linux-kernel@vger.kernel.org, linux-cifs , linux-fsdevel , Linux NFS Mailing list , wine-devel@winehq.org Subject: Re: [PATCH v3 6/7] NFSv4: Add O_DENY* open flags support Message-ID: <20130404090232.30457b32@corrin.poochiereds.net> In-Reply-To: References: <1362065133-9490-1-git-send-email-piastry@etersoft.ru> <1362065133-9490-7-git-send-email-piastry@etersoft.ru> <20130311145434.707f5ed1@corrin.poochiereds.net> <20130312083517.770a17f6@tlielax.poochiereds.net> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-nfs-owner@vger.kernel.org List-ID: On Thu, 4 Apr 2013 14:30:12 +0400 Pavel Shilovsky wrote: > 2013/3/12 Jeff Layton : > > On Mon, 11 Mar 2013 14:54:34 -0400 > > Jeff Layton wrote: > > > >> On Thu, 28 Feb 2013 19:25:32 +0400 > >> Pavel Shilovsky wrote: > >> > >> > by passing these flags to NFSv4 open request. > >> > > >> > Signed-off-by: Pavel Shilovsky > >> > --- > >> > 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 > > > > 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