On Thu, 28 Feb 2013 19:25:32 +0400
Pavel Shilovsky <[email protected]> wrote:
> by passing these flags to NFSv4 open request.
>
> Signed-off-by: Pavel Shilovsky <[email protected]>
> ---
> 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 <[email protected]>
On Mon, 11 Mar 2013 14:54:34 -0400
Jeff Layton <[email protected]> wrote:
> On Thu, 28 Feb 2013 19:25:32 +0400
> Pavel Shilovsky <[email protected]> wrote:
>
> > by passing these flags to NFSv4 open request.
> >
> > Signed-off-by: Pavel Shilovsky <[email protected]>
> > ---
> > 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 <[email protected]>
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 <[email protected]>
2013/3/12 Jeff Layton <[email protected]>:
> On Mon, 11 Mar 2013 14:54:34 -0400
> Jeff Layton <[email protected]> wrote:
>
>> On Thu, 28 Feb 2013 19:25:32 +0400
>> Pavel Shilovsky <[email protected]> wrote:
>>
>> > by passing these flags to NFSv4 open request.
>> >
>> > Signed-off-by: Pavel Shilovsky <[email protected]>
>> > ---
>> > 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 <[email protected]>
>
> 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.
On Thu, 4 Apr 2013 14:30:12 +0400
Pavel Shilovsky <[email protected]> wrote:
> 2013/3/12 Jeff Layton <[email protected]>:
> > On Mon, 11 Mar 2013 14:54:34 -0400
> > Jeff Layton <[email protected]> wrote:
> >
> >> On Thu, 28 Feb 2013 19:25:32 +0400
> >> Pavel Shilovsky <[email protected]> wrote:
> >>
> >> > by passing these flags to NFSv4 open request.
> >> >
> >> > Signed-off-by: Pavel Shilovsky <[email protected]>
> >> > ---
> >> > 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 <[email protected]>
> >
> > 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 <[email protected]>
2013/4/4 Jeff Layton <[email protected]>:
> 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.