| Message ID | 1684799560-31663-4-git-send-email-dai.ngo@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NFSD: add support for NFSv4 write delegation | expand |
On Mon, 2023-05-22 at 16:52 -0700, Dai Ngo wrote: > Remove the check for F_WRLCK in generic_add_lease to allow file_lock > to be used for write delegation. > > First consumer is NFSD. > > Signed-off-by: Dai Ngo <dai.ngo@oracle.com> > --- > fs/locks.c | 7 ------- > 1 file changed, 7 deletions(-) > > diff --git a/fs/locks.c b/fs/locks.c > index df8b26a42524..08fb0b4fd4f8 100644 > --- a/fs/locks.c > +++ b/fs/locks.c > @@ -1729,13 +1729,6 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr > if (is_deleg && !inode_trylock(inode)) > return -EAGAIN; > > - if (is_deleg && arg == F_WRLCK) { > - /* Write delegations are not currently supported: */ > - inode_unlock(inode); > - WARN_ON_ONCE(1); > - return -EINVAL; > - } > - > percpu_down_read(&file_rwsem); > spin_lock(&ctx->flc_lock); > time_out_leases(inode, &dispose); I'd probably move this back to the first patch in the series. Reviewed-by: Jeff Layton <jlayton@kernel.org>
> On May 24, 2023, at 11:08 AM, Jeff Layton <jlayton@kernel.org> wrote: > > On Mon, 2023-05-22 at 16:52 -0700, Dai Ngo wrote: >> Remove the check for F_WRLCK in generic_add_lease to allow file_lock >> to be used for write delegation. >> >> First consumer is NFSD. >> >> Signed-off-by: Dai Ngo <dai.ngo@oracle.com> >> --- >> fs/locks.c | 7 ------- >> 1 file changed, 7 deletions(-) >> >> diff --git a/fs/locks.c b/fs/locks.c >> index df8b26a42524..08fb0b4fd4f8 100644 >> --- a/fs/locks.c >> +++ b/fs/locks.c >> @@ -1729,13 +1729,6 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr >> if (is_deleg && !inode_trylock(inode)) >> return -EAGAIN; >> >> - if (is_deleg && arg == F_WRLCK) { >> - /* Write delegations are not currently supported: */ >> - inode_unlock(inode); >> - WARN_ON_ONCE(1); >> - return -EINVAL; >> - } >> - >> percpu_down_read(&file_rwsem); >> spin_lock(&ctx->flc_lock); >> time_out_leases(inode, &dispose); > > I'd probably move this back to the first patch in the series. > > Reviewed-by: Jeff Layton <jlayton@kernel.org> I asked him to move it to the end. Is it safe to take out this check before write delegation is actually implemented? -- Chuck Lever
On Wed, 2023-05-24 at 15:09 +0000, Chuck Lever III wrote: > > > On May 24, 2023, at 11:08 AM, Jeff Layton <jlayton@kernel.org> wrote: > > > > On Mon, 2023-05-22 at 16:52 -0700, Dai Ngo wrote: > > > Remove the check for F_WRLCK in generic_add_lease to allow file_lock > > > to be used for write delegation. > > > > > > First consumer is NFSD. > > > > > > Signed-off-by: Dai Ngo <dai.ngo@oracle.com> > > > --- > > > fs/locks.c | 7 ------- > > > 1 file changed, 7 deletions(-) > > > > > > diff --git a/fs/locks.c b/fs/locks.c > > > index df8b26a42524..08fb0b4fd4f8 100644 > > > --- a/fs/locks.c > > > +++ b/fs/locks.c > > > @@ -1729,13 +1729,6 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr > > > if (is_deleg && !inode_trylock(inode)) > > > return -EAGAIN; > > > > > > - if (is_deleg && arg == F_WRLCK) { > > > - /* Write delegations are not currently supported: */ > > > - inode_unlock(inode); > > > - WARN_ON_ONCE(1); > > > - return -EINVAL; > > > - } > > > - > > > percpu_down_read(&file_rwsem); > > > spin_lock(&ctx->flc_lock); > > > time_out_leases(inode, &dispose); > > > > I'd probably move this back to the first patch in the series. > > > > Reviewed-by: Jeff Layton <jlayton@kernel.org> > > I asked him to move it to the end. Is it safe to take out this > check before write delegation is actually implemented? > I think so, but it don't think it doesn't make much difference either way. The only real downside of putting it at the end is that you might have to contend with a WARN_ON_ONCE if you're bisecting.
> On May 24, 2023, at 12:55 PM, Jeff Layton <jlayton@kernel.org> wrote: > > On Wed, 2023-05-24 at 15:09 +0000, Chuck Lever III wrote: >> >>> On May 24, 2023, at 11:08 AM, Jeff Layton <jlayton@kernel.org> wrote: >>> >>> On Mon, 2023-05-22 at 16:52 -0700, Dai Ngo wrote: >>>> Remove the check for F_WRLCK in generic_add_lease to allow file_lock >>>> to be used for write delegation. >>>> >>>> First consumer is NFSD. >>>> >>>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com> >>>> --- >>>> fs/locks.c | 7 ------- >>>> 1 file changed, 7 deletions(-) >>>> >>>> diff --git a/fs/locks.c b/fs/locks.c >>>> index df8b26a42524..08fb0b4fd4f8 100644 >>>> --- a/fs/locks.c >>>> +++ b/fs/locks.c >>>> @@ -1729,13 +1729,6 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr >>>> if (is_deleg && !inode_trylock(inode)) >>>> return -EAGAIN; >>>> >>>> - if (is_deleg && arg == F_WRLCK) { >>>> - /* Write delegations are not currently supported: */ >>>> - inode_unlock(inode); >>>> - WARN_ON_ONCE(1); >>>> - return -EINVAL; >>>> - } >>>> - >>>> percpu_down_read(&file_rwsem); >>>> spin_lock(&ctx->flc_lock); >>>> time_out_leases(inode, &dispose); >>> >>> I'd probably move this back to the first patch in the series. >>> >>> Reviewed-by: Jeff Layton <jlayton@kernel.org> >> >> I asked him to move it to the end. Is it safe to take out this >> check before write delegation is actually implemented? >> > > I think so, but it don't think it doesn't make much difference either > way. The only real downside of putting it at the end is that you might > have to contend with a WARN_ON_ONCE if you're bisecting. My main concern is in fact preventing problems during bisection. I can apply 3/3 and then 1/3, if you're good with that. -- Chuck Lever
On 5/24/23 9:55 AM, Jeff Layton wrote: > On Wed, 2023-05-24 at 15:09 +0000, Chuck Lever III wrote: >>> On May 24, 2023, at 11:08 AM, Jeff Layton <jlayton@kernel.org> wrote: >>> >>> On Mon, 2023-05-22 at 16:52 -0700, Dai Ngo wrote: >>>> Remove the check for F_WRLCK in generic_add_lease to allow file_lock >>>> to be used for write delegation. >>>> >>>> First consumer is NFSD. >>>> >>>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com> >>>> --- >>>> fs/locks.c | 7 ------- >>>> 1 file changed, 7 deletions(-) >>>> >>>> diff --git a/fs/locks.c b/fs/locks.c >>>> index df8b26a42524..08fb0b4fd4f8 100644 >>>> --- a/fs/locks.c >>>> +++ b/fs/locks.c >>>> @@ -1729,13 +1729,6 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr >>>> if (is_deleg && !inode_trylock(inode)) >>>> return -EAGAIN; >>>> >>>> - if (is_deleg && arg == F_WRLCK) { >>>> - /* Write delegations are not currently supported: */ >>>> - inode_unlock(inode); >>>> - WARN_ON_ONCE(1); >>>> - return -EINVAL; >>>> - } >>>> - >>>> percpu_down_read(&file_rwsem); >>>> spin_lock(&ctx->flc_lock); >>>> time_out_leases(inode, &dispose); >>> I'd probably move this back to the first patch in the series. >>> >>> Reviewed-by: Jeff Layton <jlayton@kernel.org> >> I asked him to move it to the end. Is it safe to take out this >> check before write delegation is actually implemented? >> > I think so, but it don't think it doesn't make much difference either > way. The only real downside of putting it at the end is that you might > have to contend with a WARN_ON_ONCE if you're bisecting. I will make this patch to be the 1st patch, we don't want the user to get WARN_ON_ONCE when bisecting. -Dai
On 5/24/23 10:41 AM, Chuck Lever III wrote: > >> On May 24, 2023, at 12:55 PM, Jeff Layton <jlayton@kernel.org> wrote: >> >> On Wed, 2023-05-24 at 15:09 +0000, Chuck Lever III wrote: >>>> On May 24, 2023, at 11:08 AM, Jeff Layton <jlayton@kernel.org> wrote: >>>> >>>> On Mon, 2023-05-22 at 16:52 -0700, Dai Ngo wrote: >>>>> Remove the check for F_WRLCK in generic_add_lease to allow file_lock >>>>> to be used for write delegation. >>>>> >>>>> First consumer is NFSD. >>>>> >>>>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com> >>>>> --- >>>>> fs/locks.c | 7 ------- >>>>> 1 file changed, 7 deletions(-) >>>>> >>>>> diff --git a/fs/locks.c b/fs/locks.c >>>>> index df8b26a42524..08fb0b4fd4f8 100644 >>>>> --- a/fs/locks.c >>>>> +++ b/fs/locks.c >>>>> @@ -1729,13 +1729,6 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr >>>>> if (is_deleg && !inode_trylock(inode)) >>>>> return -EAGAIN; >>>>> >>>>> - if (is_deleg && arg == F_WRLCK) { >>>>> - /* Write delegations are not currently supported: */ >>>>> - inode_unlock(inode); >>>>> - WARN_ON_ONCE(1); >>>>> - return -EINVAL; >>>>> - } >>>>> - >>>>> percpu_down_read(&file_rwsem); >>>>> spin_lock(&ctx->flc_lock); >>>>> time_out_leases(inode, &dispose); >>>> I'd probably move this back to the first patch in the series. >>>> >>>> Reviewed-by: Jeff Layton <jlayton@kernel.org> >>> I asked him to move it to the end. Is it safe to take out this >>> check before write delegation is actually implemented? >>> >> I think so, but it don't think it doesn't make much difference either >> way. The only real downside of putting it at the end is that you might >> have to contend with a WARN_ON_ONCE if you're bisecting. > My main concern is in fact preventing problems during bisection. > I can apply 3/3 and then 1/3, if you're good with that. I'm good with that. You can apply 3/3 then 1/3 and drop 2/3 so I don't have to send out v6. -Dai > > > -- > Chuck Lever > >
On Wed, 2023-05-24 at 11:05 -0700, dai.ngo@oracle.com wrote: > On 5/24/23 10:41 AM, Chuck Lever III wrote: > > > > > On May 24, 2023, at 12:55 PM, Jeff Layton <jlayton@kernel.org> wrote: > > > > > > On Wed, 2023-05-24 at 15:09 +0000, Chuck Lever III wrote: > > > > > On May 24, 2023, at 11:08 AM, Jeff Layton <jlayton@kernel.org> wrote: > > > > > > > > > > On Mon, 2023-05-22 at 16:52 -0700, Dai Ngo wrote: > > > > > > Remove the check for F_WRLCK in generic_add_lease to allow file_lock > > > > > > to be used for write delegation. > > > > > > > > > > > > First consumer is NFSD. > > > > > > > > > > > > Signed-off-by: Dai Ngo <dai.ngo@oracle.com> > > > > > > --- > > > > > > fs/locks.c | 7 ------- > > > > > > 1 file changed, 7 deletions(-) > > > > > > > > > > > > diff --git a/fs/locks.c b/fs/locks.c > > > > > > index df8b26a42524..08fb0b4fd4f8 100644 > > > > > > --- a/fs/locks.c > > > > > > +++ b/fs/locks.c > > > > > > @@ -1729,13 +1729,6 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr > > > > > > if (is_deleg && !inode_trylock(inode)) > > > > > > return -EAGAIN; > > > > > > > > > > > > - if (is_deleg && arg == F_WRLCK) { > > > > > > - /* Write delegations are not currently supported: */ > > > > > > - inode_unlock(inode); > > > > > > - WARN_ON_ONCE(1); > > > > > > - return -EINVAL; > > > > > > - } > > > > > > - > > > > > > percpu_down_read(&file_rwsem); > > > > > > spin_lock(&ctx->flc_lock); > > > > > > time_out_leases(inode, &dispose); > > > > > I'd probably move this back to the first patch in the series. > > > > > > > > > > Reviewed-by: Jeff Layton <jlayton@kernel.org> > > > > I asked him to move it to the end. Is it safe to take out this > > > > check before write delegation is actually implemented? > > > > > > > I think so, but it don't think it doesn't make much difference either > > > way. The only real downside of putting it at the end is that you might > > > have to contend with a WARN_ON_ONCE if you're bisecting. > > My main concern is in fact preventing problems during bisection. > > I can apply 3/3 and then 1/3, if you're good with that. > > I'm good with that. You can apply 3/3 then 1/3 and drop 2/3 so I > don't have to send out v6. > I'm fine with that too, particularly if other vendors don't recall on a getattr currently. I wonder though, if we need some clarification in the spec on CB_GETATTR? https://www.rfc-editor.org/rfc/rfc8881.html#section-10.4.3 In that section, there is a big distinction about the client being able to tell that the data was modified from the point where the delegation was handed out. There is always a point in time where a client has buffered writes that haven't been flushed to the server yet, but that's true when it doesn't have a delegation too. Mostly the client tries to start some writeback fairly quickly so any lag how the in the change attr/size update is usually short lived. I don't think the Linux client materially changes its writeback behavior based on a write delegation, so I guess (as Olga pointed out) the main benefit from a write delegation is being able to do delegated opens for write. A getattr's results won't be changed by extra opens or closes, so yeah...I guess the utility of CB_GETATTR is really limited. I guess it _might_ be useful in the case where the server has handed out a write delegation, but hasn't gotten any writes. That would at least tell the client that something has changed, even if the deleg holder hasn't gotten around to writing anything back yet. The problem is that it's common for applications to open O_RDWR and only do reads. Maybe we ought to take this discussion to the IETF list? It seems like the spec mandates that you must recall the delegation if you don't implement CB_GETATTR, but I don't see much in way of harm in ignoring that.
On 5/24/23 12:03 PM, Jeff Layton wrote: > On Wed, 2023-05-24 at 11:05 -0700, dai.ngo@oracle.com wrote: >> On 5/24/23 10:41 AM, Chuck Lever III wrote: >>>> On May 24, 2023, at 12:55 PM, Jeff Layton <jlayton@kernel.org> wrote: >>>> >>>> On Wed, 2023-05-24 at 15:09 +0000, Chuck Lever III wrote: >>>>>> On May 24, 2023, at 11:08 AM, Jeff Layton <jlayton@kernel.org> wrote: >>>>>> >>>>>> On Mon, 2023-05-22 at 16:52 -0700, Dai Ngo wrote: >>>>>>> Remove the check for F_WRLCK in generic_add_lease to allow file_lock >>>>>>> to be used for write delegation. >>>>>>> >>>>>>> First consumer is NFSD. >>>>>>> >>>>>>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com> >>>>>>> --- >>>>>>> fs/locks.c | 7 ------- >>>>>>> 1 file changed, 7 deletions(-) >>>>>>> >>>>>>> diff --git a/fs/locks.c b/fs/locks.c >>>>>>> index df8b26a42524..08fb0b4fd4f8 100644 >>>>>>> --- a/fs/locks.c >>>>>>> +++ b/fs/locks.c >>>>>>> @@ -1729,13 +1729,6 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr >>>>>>> if (is_deleg && !inode_trylock(inode)) >>>>>>> return -EAGAIN; >>>>>>> >>>>>>> - if (is_deleg && arg == F_WRLCK) { >>>>>>> - /* Write delegations are not currently supported: */ >>>>>>> - inode_unlock(inode); >>>>>>> - WARN_ON_ONCE(1); >>>>>>> - return -EINVAL; >>>>>>> - } >>>>>>> - >>>>>>> percpu_down_read(&file_rwsem); >>>>>>> spin_lock(&ctx->flc_lock); >>>>>>> time_out_leases(inode, &dispose); >>>>>> I'd probably move this back to the first patch in the series. >>>>>> >>>>>> Reviewed-by: Jeff Layton <jlayton@kernel.org> >>>>> I asked him to move it to the end. Is it safe to take out this >>>>> check before write delegation is actually implemented? >>>>> >>>> I think so, but it don't think it doesn't make much difference either >>>> way. The only real downside of putting it at the end is that you might >>>> have to contend with a WARN_ON_ONCE if you're bisecting. >>> My main concern is in fact preventing problems during bisection. >>> I can apply 3/3 and then 1/3, if you're good with that. >> I'm good with that. You can apply 3/3 then 1/3 and drop 2/3 so I >> don't have to send out v6. >> > I'm fine with that too, particularly if other vendors don't recall on a > getattr currently. > > I wonder though, if we need some clarification in the spec on > CB_GETATTR? > > https://www.rfc-editor.org/rfc/rfc8881.html#section-10.4.3 > > In that section, there is a big distinction about the client being able > to tell that the data was modified from the point where the delegation > was handed out. > > There is always a point in time where a client has buffered writes that > haven't been flushed to the server yet, but that's true when it doesn't > have a delegation too. Mostly the client tries to start some writeback > fairly quickly so any lag how the in the change attr/size update is > usually short lived. > > I don't think the Linux client materially changes its writeback behavior > based on a write delegation, so I guess (as Olga pointed out) the main > benefit from a write delegation is being able to do delegated opens for > write. A getattr's results won't be changed by extra opens or closes, so > yeah...I guess the utility of CB_GETATTR is really limited. > > I guess it _might_ be useful in the case where the server has handed out > a write delegation, but hasn't gotten any writes. That would at least > tell the client that something has changed, even if the deleg holder > hasn't gotten around to writing anything back yet. The problem is that > it's common for applications to open O_RDWR and only do reads. > > Maybe we ought to take this discussion to the IETF list? It seems like > the spec mandates that you must recall the delegation if you don't > implement CB_GETATTR, but I don't see much in way of harm in ignoring > that. Yes, I think we should, for clarification. Jeff, would you mind to drive this discussion on IETF since you can present the issue much clearer than I would. Thanks, -Dai
diff --git a/fs/locks.c b/fs/locks.c index df8b26a42524..08fb0b4fd4f8 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -1729,13 +1729,6 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr if (is_deleg && !inode_trylock(inode)) return -EAGAIN; - if (is_deleg && arg == F_WRLCK) { - /* Write delegations are not currently supported: */ - inode_unlock(inode); - WARN_ON_ONCE(1); - return -EINVAL; - } - percpu_down_read(&file_rwsem); spin_lock(&ctx->flc_lock); time_out_leases(inode, &dispose);
Remove the check for F_WRLCK in generic_add_lease to allow file_lock to be used for write delegation. First consumer is NFSD. Signed-off-by: Dai Ngo <dai.ngo@oracle.com> --- fs/locks.c | 7 ------- 1 file changed, 7 deletions(-)