| Message ID | 1362065133-9490-7-git-send-email-piastry@etersoft.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 28 Feb 2013 19:25:32 +0400 Pavel Shilovsky <piastry@etersoft.ru> wrote: > by passing these flags to NFSv4 open request. > > Signed-off-by: Pavel Shilovsky <piastry@etersoft.ru> > --- > 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@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 11 Mar 2013 14:54:34 -0400 Jeff Layton <jlayton@redhat.com> wrote: > On Thu, 28 Feb 2013 19:25:32 +0400 > Pavel Shilovsky <piastry@etersoft.ru> wrote: > > > by passing these flags to NFSv4 open request. > > > > Signed-off-by: Pavel Shilovsky <piastry@etersoft.ru> > > --- > > 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@redhat.com> 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?
2013/3/12 Jeff Layton <jlayton@redhat.com>: > On Mon, 11 Mar 2013 14:54:34 -0400 > Jeff Layton <jlayton@redhat.com> wrote: > >> On Thu, 28 Feb 2013 19:25:32 +0400 >> Pavel Shilovsky <piastry@etersoft.ru> wrote: >> >> > by passing these flags to NFSv4 open request. >> > >> > Signed-off-by: Pavel Shilovsky <piastry@etersoft.ru> >> > --- >> > 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@redhat.com> > > 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? -- Best regards, Pavel Shilovsky. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 4 Apr 2013 14:30:12 +0400 Pavel Shilovsky <piastry@etersoft.ru> wrote: > 2013/3/12 Jeff Layton <jlayton@redhat.com>: > > On Mon, 11 Mar 2013 14:54:34 -0400 > > Jeff Layton <jlayton@redhat.com> wrote: > > > >> On Thu, 28 Feb 2013 19:25:32 +0400 > >> Pavel Shilovsky <piastry@etersoft.ru> wrote: > >> > >> > by passing these flags to NFSv4 open request. > >> > > >> > Signed-off-by: Pavel Shilovsky <piastry@etersoft.ru> > >> > --- > >> > 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@redhat.com> > > > > 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...
2013/4/4 Jeff Layton <jlayton@redhat.com>: > 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... Ok, this makes sense, thanks. Will do it this way and repost. -- Best regards, Pavel Shilovsky. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) { + 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
by passing these flags to NFSv4 open request. Signed-off-by: Pavel Shilovsky <piastry@etersoft.ru> --- fs/nfs/nfs4xdr.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-)