Re: [PATCH v3 7/7] NFSD: Pass share reservations flags to VFS
"J. Bruce Fields" wrote> > > > Another bit to consider, does lockd provide share reservations for NLM? > > Yes. I don't know if anyone's tested them in recent memory! But it > might be interesting to write a few simple tests for them and hook them > up to this on the server side. (I don't know if they'd be worth > implementing on the client side?) Oh, they would be REALLY NICE to have on the client side... Then we can test NLM_SHARE with an open source client and not only with Windows... Frank
Re: [PATCH v3 7/7] NFSD: Pass share reservations flags to VFS
On Mon, Mar 11, 2013 at 01:25:20PM -0700, Frank S Filz wrote:
>
> "J. Bruce Fields" wrote
> > On Mon, Mar 11, 2013 at 04:08:44PM -0400, Jeff Layton wrote:
> > > On Mon, 11 Mar 2013 15:36:38 -0400
> > > "J. Bruce Fields" wrote:
> > > > > It doesn't look like this patch removes any of that old code
> though. I
> > > > > think it probably should, or there ought to be some consideration
> of
> > > > > how this new stuff will mesh with it.
> > > > >
> > > > > I think you have 2 choices here:
> > > > >
> > > > > 1/ rip out the old share reservation code altogether and require
> that
> > > > > filesystems mount with -o sharemand or whatever if they want to
> allow
> > > > > their enforcement
> > > > >
> > > > > 2/ make knfsd fall back to using the internal share reservation
> code
> > > > > when the mount option isn't enabled
> > > > >
> > > > > Personally, I think #1 would be fine, but Bruce may want to weigh
> in on
> > > > > what he'd prefer.
> > > >
> > > > #1 sounds good. Clients that use deny bits are few. My preference
> > > > would be to return an error to such clients in the case share locks
> > > > aren't available.
> > > >
> > > > (We're a little out of spec there, so I'm not sure which error. I
> think
> > > > the goal is to notify a human there's a problem with minimal
> collateral
> > > > damange.
> > > >
> > > > NFS4ERR_SERVERFAULT ("I'm a buggy server, sorry about that!") would
> > > > probably result in an IO error to the application.
> > > >
> > > > SHARE_DENIED strikes me as unsafe: an application would be in its
> rights
> > > > not to even check for that e.g. in the case of an exclusive create.
>
> Hmm, shouldn't the client catch that with a "default" case at least?
>
> > > > Maybe DELAY? Kind of ridiculous, but blocking the application
> > > > indefinitely would probably get someone's attention quickly enough
> > > > without doing any damnage.)
> > > >
> > >
> > > I agree that we should return an error, but hadn't considered what
> > > error. Given that hardly any NFS clients use them, I'd probably just go
> > > with NFS4ERR_SERVERFAULT, and maybe throw a printk or something on the
> > > server about enabling share reservations for superblock x:y.
> >
> > Sounds reasonable.
>
> If I'm understanding, the suggestion is a mount option to enable share
> reservations and if so, they will be mandatory?
>
> In that case, perhaps we want to keep the existing knfsd code as a
> fallback, someone might want to support them, but not have them be
> mandatory (if nothing else, you may cause consternation from folks running
> pynfs against a default configured knfsd server).
Understood, but the benefit is slight and the cost (in complexity) is
rather large. On balance I'd far prefer to get rid of any fallback code
entirely.
> In the Ganesha project, we provide an internal implementation of share
> reservations for when the underlying system can not support them.
>
> Another bit to consider, does lockd provide share reservations for NLM?
Yes. I don't know if anyone's tested them in recent memory! But it
might be interesting to write a few simple tests for them and hook them
up to this on the server side. (I don't know if they'd be worth
implementing on the client side?)
--b.
Re: [PATCH v3 7/7] NFSD: Pass share reservations flags to VFS
"J. Bruce Fields" wrote
> On Mon, Mar 11, 2013 at 04:08:44PM -0400, Jeff Layton wrote:
> > On Mon, 11 Mar 2013 15:36:38 -0400
> > "J. Bruce Fields" wrote:
> > > > It doesn't look like this patch removes any of that old code
though. I
> > > > think it probably should, or there ought to be some consideration
of
> > > > how this new stuff will mesh with it.
> > > >
> > > > I think you have 2 choices here:
> > > >
> > > > 1/ rip out the old share reservation code altogether and require
that
> > > > filesystems mount with -o sharemand or whatever if they want to
allow
> > > > their enforcement
> > > >
> > > > 2/ make knfsd fall back to using the internal share reservation
code
> > > > when the mount option isn't enabled
> > > >
> > > > Personally, I think #1 would be fine, but Bruce may want to weigh
in on
> > > > what he'd prefer.
> > >
> > > #1 sounds good. Clients that use deny bits are few. My preference
> > > would be to return an error to such clients in the case share locks
> > > aren't available.
> > >
> > > (We're a little out of spec there, so I'm not sure which error. I
think
> > > the goal is to notify a human there's a problem with minimal
collateral
> > > damange.
> > >
> > > NFS4ERR_SERVERFAULT ("I'm a buggy server, sorry about that!") would
> > > probably result in an IO error to the application.
> > >
> > > SHARE_DENIED strikes me as unsafe: an application would be in its
rights
> > > not to even check for that e.g. in the case of an exclusive create.
Hmm, shouldn't the client catch that with a "default" case at least?
> > > Maybe DELAY? Kind of ridiculous, but blocking the application
> > > indefinitely would probably get someone's attention quickly enough
> > > without doing any damnage.)
> > >
> >
> > I agree that we should return an error, but hadn't considered what
> > error. Given that hardly any NFS clients use them, I'd probably just go
> > with NFS4ERR_SERVERFAULT, and maybe throw a printk or something on the
> > server about enabling share reservations for superblock x:y.
>
> Sounds reasonable.
If I'm understanding, the suggestion is a mount option to enable share
reservations and if so, they will be mandatory?
In that case, perhaps we want to keep the existing knfsd code as a
fallback, someone might want to support them, but not have them be
mandatory (if nothing else, you may cause consternation from folks running
pynfs against a default configured knfsd server).
In the Ganesha project, we provide an internal implementation of share
reservations for when the underlying system can not support them.
Another bit to consider, does lockd provide share reservations for NLM?
Frank
Re: [PATCH v3 7/7] NFSD: Pass share reservations flags to VFS
On Mon, Mar 11, 2013 at 04:08:44PM -0400, Jeff Layton wrote:
> On Mon, 11 Mar 2013 15:36:38 -0400
> "J. Bruce Fields" wrote:
>
> > On Mon, Mar 11, 2013 at 03:05:40PM -0400, Jeff Layton wrote:
> > > knfsd has some code already to handle share reservations internally.
> > > Nothing outside of knfsd is aware of these reservations, of course so
> > > moving to a vfs-level object for it would be a marked improvement.
> > >
> > > It doesn't look like this patch removes any of that old code though. I
> > > think it probably should, or there ought to be some consideration of
> > > how this new stuff will mesh with it.
> > >
> > > I think you have 2 choices here:
> > >
> > > 1/ rip out the old share reservation code altogether and require that
> > > filesystems mount with -o sharemand or whatever if they want to allow
> > > their enforcement
> > >
> > > 2/ make knfsd fall back to using the internal share reservation code
> > > when the mount option isn't enabled
> > >
> > > Personally, I think #1 would be fine, but Bruce may want to weigh in on
> > > what he'd prefer.
> >
> > #1 sounds good. Clients that use deny bits are few. My preference
> > would be to return an error to such clients in the case share locks
> > aren't available.
> >
> > (We're a little out of spec there, so I'm not sure which error. I think
> > the goal is to notify a human there's a problem with minimal collateral
> > damange.
> >
> > NFS4ERR_SERVERFAULT ("I'm a buggy server, sorry about that!") would
> > probably result in an IO error to the application.
> >
> > SHARE_DENIED strikes me as unsafe: an application would be in its rights
> > not to even check for that e.g. in the case of an exclusive create.
> >
> > Maybe DELAY? Kind of ridiculous, but blocking the application
> > indefinitely would probably get someone's attention quickly enough
> > without doing any damnage.)
> >
>
> I agree that we should return an error, but hadn't considered what
> error. Given that hardly any NFS clients use them, I'd probably just go
> with NFS4ERR_SERVERFAULT, and maybe throw a printk or something on the
> server about enabling share reservations for superblock x:y.
Sounds reasonable.
> Pavel, as a side note, you may want to consider adding a patch to hook
> this stuff up in the NFS client as well.
Definitely.
--b.
Re: [PATCH v3 7/7] NFSD: Pass share reservations flags to VFS
On Mon, 11 Mar 2013 15:36:38 -0400
"J. Bruce Fields" wrote:
> On Mon, Mar 11, 2013 at 03:05:40PM -0400, Jeff Layton wrote:
> > knfsd has some code already to handle share reservations internally.
> > Nothing outside of knfsd is aware of these reservations, of course so
> > moving to a vfs-level object for it would be a marked improvement.
> >
> > It doesn't look like this patch removes any of that old code though. I
> > think it probably should, or there ought to be some consideration of
> > how this new stuff will mesh with it.
> >
> > I think you have 2 choices here:
> >
> > 1/ rip out the old share reservation code altogether and require that
> > filesystems mount with -o sharemand or whatever if they want to allow
> > their enforcement
> >
> > 2/ make knfsd fall back to using the internal share reservation code
> > when the mount option isn't enabled
> >
> > Personally, I think #1 would be fine, but Bruce may want to weigh in on
> > what he'd prefer.
>
> #1 sounds good. Clients that use deny bits are few. My preference
> would be to return an error to such clients in the case share locks
> aren't available.
>
> (We're a little out of spec there, so I'm not sure which error. I think
> the goal is to notify a human there's a problem with minimal collateral
> damange.
>
> NFS4ERR_SERVERFAULT ("I'm a buggy server, sorry about that!") would
> probably result in an IO error to the application.
>
> SHARE_DENIED strikes me as unsafe: an application would be in its rights
> not to even check for that e.g. in the case of an exclusive create.
>
> Maybe DELAY? Kind of ridiculous, but blocking the application
> indefinitely would probably get someone's attention quickly enough
> without doing any damnage.)
>
I agree that we should return an error, but hadn't considered what
error. Given that hardly any NFS clients use them, I'd probably just go
with NFS4ERR_SERVERFAULT, and maybe throw a printk or something on the
server about enabling share reservations for superblock x:y.
Pavel, as a side note, you may want to consider adding a patch to hook
this stuff up in the NFS client as well.
--
Jeff Layton
Re: [PATCH v3 7/7] NFSD: Pass share reservations flags to VFS
On Mon, Mar 11, 2013 at 03:05:40PM -0400, Jeff Layton wrote:
> knfsd has some code already to handle share reservations internally.
> Nothing outside of knfsd is aware of these reservations, of course so
> moving to a vfs-level object for it would be a marked improvement.
>
> It doesn't look like this patch removes any of that old code though. I
> think it probably should, or there ought to be some consideration of
> how this new stuff will mesh with it.
>
> I think you have 2 choices here:
>
> 1/ rip out the old share reservation code altogether and require that
> filesystems mount with -o sharemand or whatever if they want to allow
> their enforcement
>
> 2/ make knfsd fall back to using the internal share reservation code
> when the mount option isn't enabled
>
> Personally, I think #1 would be fine, but Bruce may want to weigh in on
> what he'd prefer.
#1 sounds good. Clients that use deny bits are few. My preference
would be to return an error to such clients in the case share locks
aren't available.
(We're a little out of spec there, so I'm not sure which error. I think
the goal is to notify a human there's a problem with minimal collateral
damange.
NFS4ERR_SERVERFAULT ("I'm a buggy server, sorry about that!") would
probably result in an IO error to the application.
SHARE_DENIED strikes me as unsafe: an application would be in its rights
not to even check for that e.g. in the case of an exclusive create.
Maybe DELAY? Kind of ridiculous, but blocking the application
indefinitely would probably get someone's attention quickly enough
without doing any damnage.)
--b.
Re: [PATCH v3 7/7] NFSD: Pass share reservations flags to VFS
On Thu, 28 Feb 2013 19:25:33 +0400
Pavel Shilovsky wrote:
> that maps them into O_DENY flags and make them visible for
> applications that use O_DENYMAND opens.
>
> Signed-off-by: Pavel Shilovsky
> ---
> fs/locks.c | 1 +
> fs/nfsd/nfs4state.c | 46 +-
> 2 files changed, 46 insertions(+), 1 deletion(-)
>
> diff --git a/fs/locks.c b/fs/locks.c
> index 0cc7d1b..593d464 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -874,6 +874,7 @@ deny_lock_file(struct file *filp)
> locks_free_lock(lock);
> return error;
> }
> +EXPORT_SYMBOL(deny_lock_file);
>
> static int __posix_lock_file(struct inode *inode, struct file_lock *request,
> struct file_lock *conflock)
> {
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index ac8ed96c..766256a 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -476,6 +476,19 @@ test_deny(u32 access, struct nfs4_ol_stateid *stp)
> return test_bit(access, &stp->st_deny_bmap);
> }
>
> +static int nfs4_deny_to_odeny(u32 access)
> +{
> + switch (access & NFS4_SHARE_DENY_BOTH) {
> + case NFS4_SHARE_DENY_READ:
> + return O_DENYMAND | O_DENYREAD;
> + case NFS4_SHARE_DENY_WRITE:
> + return O_DENYWRITE | O_DENYMAND;
> + case NFS4_SHARE_DENY_BOTH:
> + return O_DENYREAD | O_DENYWRITE | O_DENYMAND;
> + }
> + return O_DENYMAND;
> +}
> +
> static int nfs4_access_to_omode(u32 access)
> {
> switch (access & NFS4_SHARE_ACCESS_BOTH) {
> @@ -2793,6 +2806,21 @@ nfsd4_truncate(struct svc_rqst *rqstp, struct svc_fh
> *fh,
> }
>
> static __be32
> +nfs4_vfs_set_deny(struct nfs4_file *fp, unsigned long share_access,
> + unsigned long deny_access)
> +{
> + int oflag, rc;
> + __be32 status = nfs_ok;
> +
> + oflag = nfs4_access_to_omode(share_access);
> + fp->fi_fds[oflag]->f_flags |= nfs4_deny_to_odeny(deny_access);
> + rc = deny_lock_file(fp->fi_fds[oflag]);
> + if (rc)
> + status = nfserrno(rc);
> + return status;
> +}
> +
> +static __be32
> nfs4_upgrade_open(struct svc_rqst *rqstp, struct nfs4_file *fp, struct
> svc_fh *cur_fh, struct nfs4_ol_stateid *stp, struct nfsd4_open *open)
> {
> u32 op_share_access = open->op_share_access;
> @@ -2813,6 +2841,14 @@ nfs4_upgrade_open(struct svc_rqst *rqstp, struct
> nfs4_file *fp, struct svc_fh *c
> }
> return status;
> }
> + status = nfs4_vfs_set_deny(fp, op_share_access, open->op_share_deny);
> + if (status) {
> + if (new_access) {
> + int oflag = nfs4_access_to_omode(op_share_access);
> + nfs4_file_put_access(fp, oflag);
> + }
> + return status;
> + }
> /* remember the open */
> set_access(op_share_access, stp);
> set_deny(open->op_share_deny, stp);
> @@ -3046,7 +3082,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct
> svc_fh *current_fh, struct nf
>
> /*
>* OPEN the file, or upgrade an existing OPEN.
> - * If truncate fails, the OPEN fails.
> + * If truncate or setting deny fails, the OPEN fails.
>*/
> if (stp) {
> /* Stateid was found, this is an OPEN upgrade */
> @@ -3060,6 +3096,10 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct
> svc_fh *current_fh, struct nf
> status = nfsd4_truncate(rqstp, current_fh, open);
> if (status)
> goto out;
> + status = nfs4_vfs_set_deny(fp, open->op_share_access,
> +open->op_share_deny);
> + if (status)
> + goto out;
> stp = open->op_stp;
> open->op_stp = NULL;
> init_open_stateid(stp, fp, open);
> @@ -3758,6 +3798,10 @@ nfsd4_open_downgrade(struct svc_rqst *rqstp,
> }
> nfs4_stateid_downgrade(stp, od->od_share_access);
>
> + status = nfs4_vfs_set_deny(stp->st_file, od->od_share_access,
> +od->od_share_deny);
> + if (status)
> + goto out;
> reset_union_bmap_deny(od->od_share_deny, stp);
>
> update_stateid(&stp->st_stid.sc_stateid);
knfsd has some code already to handle share reservations internally.
Nothing outside of knfsd is aware of these reservations, of course so
moving to a vfs-level object for it would be a marked improvement.
It doesn't look like this patch removes any of that old code though. I
think it probably should, or there ought to be some consideration of
how this new stuff will mesh with it.
I think you have 2 choices here:
1/ rip out the old share reservation code altogether and require that
filesystems mount with -o sharemand or whatever if they want to allow
their enforcement
2/ make knfsd fall back to using the internal share reservation code
when the mount option isn't enabled
Personally, I think #1 would be
[PATCH v3 7/7] NFSD: Pass share reservations flags to VFS
that maps them into O_DENY flags and make them visible for
applications that use O_DENYMAND opens.
Signed-off-by: Pavel Shilovsky
---
fs/locks.c | 1 +
fs/nfsd/nfs4state.c | 46 +-
2 files changed, 46 insertions(+), 1 deletion(-)
diff --git a/fs/locks.c b/fs/locks.c
index 0cc7d1b..593d464 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -874,6 +874,7 @@ deny_lock_file(struct file *filp)
locks_free_lock(lock);
return error;
}
+EXPORT_SYMBOL(deny_lock_file);
static int __posix_lock_file(struct inode *inode, struct file_lock *request,
struct file_lock *conflock)
{
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index ac8ed96c..766256a 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -476,6 +476,19 @@ test_deny(u32 access, struct nfs4_ol_stateid *stp)
return test_bit(access, &stp->st_deny_bmap);
}
+static int nfs4_deny_to_odeny(u32 access)
+{
+ switch (access & NFS4_SHARE_DENY_BOTH) {
+ case NFS4_SHARE_DENY_READ:
+ return O_DENYMAND | O_DENYREAD;
+ case NFS4_SHARE_DENY_WRITE:
+ return O_DENYWRITE | O_DENYMAND;
+ case NFS4_SHARE_DENY_BOTH:
+ return O_DENYREAD | O_DENYWRITE | O_DENYMAND;
+ }
+ return O_DENYMAND;
+}
+
static int nfs4_access_to_omode(u32 access)
{
switch (access & NFS4_SHARE_ACCESS_BOTH) {
@@ -2793,6 +2806,21 @@ nfsd4_truncate(struct svc_rqst *rqstp, struct svc_fh *fh,
}
static __be32
+nfs4_vfs_set_deny(struct nfs4_file *fp, unsigned long share_access,
+ unsigned long deny_access)
+{
+ int oflag, rc;
+ __be32 status = nfs_ok;
+
+ oflag = nfs4_access_to_omode(share_access);
+ fp->fi_fds[oflag]->f_flags |= nfs4_deny_to_odeny(deny_access);
+ rc = deny_lock_file(fp->fi_fds[oflag]);
+ if (rc)
+ status = nfserrno(rc);
+ return status;
+}
+
+static __be32
nfs4_upgrade_open(struct svc_rqst *rqstp, struct nfs4_file *fp, struct svc_fh
*cur_fh, struct nfs4_ol_stateid *stp, struct nfsd4_open *open)
{
u32 op_share_access = open->op_share_access;
@@ -2813,6 +2841,14 @@ nfs4_upgrade_open(struct svc_rqst *rqstp, struct
nfs4_file *fp, struct svc_fh *c
}
return status;
}
+ status = nfs4_vfs_set_deny(fp, op_share_access, open->op_share_deny);
+ if (status) {
+ if (new_access) {
+ int oflag = nfs4_access_to_omode(op_share_access);
+ nfs4_file_put_access(fp, oflag);
+ }
+ return status;
+ }
/* remember the open */
set_access(op_share_access, stp);
set_deny(open->op_share_deny, stp);
@@ -3046,7 +3082,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh
*current_fh, struct nf
/*
* OPEN the file, or upgrade an existing OPEN.
-* If truncate fails, the OPEN fails.
+* If truncate or setting deny fails, the OPEN fails.
*/
if (stp) {
/* Stateid was found, this is an OPEN upgrade */
@@ -3060,6 +3096,10 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct
svc_fh *current_fh, struct nf
status = nfsd4_truncate(rqstp, current_fh, open);
if (status)
goto out;
+ status = nfs4_vfs_set_deny(fp, open->op_share_access,
+ open->op_share_deny);
+ if (status)
+ goto out;
stp = open->op_stp;
open->op_stp = NULL;
init_open_stateid(stp, fp, open);
@@ -3758,6 +3798,10 @@ nfsd4_open_downgrade(struct svc_rqst *rqstp,
}
nfs4_stateid_downgrade(stp, od->od_share_access);
+ status = nfs4_vfs_set_deny(stp->st_file, od->od_share_access,
+ od->od_share_deny);
+ if (status)
+ goto out;
reset_union_bmap_deny(od->od_share_deny, stp);
update_stateid(&stp->st_stid.sc_stateid);
--
1.8.1.2
