Its possible for __break_lease to find the layout's lease before we've
added the layout to the owner's ls_layouts list. In that case, setting
ls_recalled = true without actually recalling the layout will cause the
server to never send a recall callback.
Move the check for ls_layouts before setting ls_recalled.
Signed-off-by: Benjamin Coddington <[email protected]>
---
fs/nfsd/nfs4layouts.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
index 3564d1c6f610..e8a80052cb1b 100644
--- a/fs/nfsd/nfs4layouts.c
+++ b/fs/nfsd/nfs4layouts.c
@@ -323,11 +323,11 @@ nfsd4_recall_file_layout(struct nfs4_layout_stateid *ls)
if (ls->ls_recalled)
goto out_unlock;
- ls->ls_recalled = true;
- atomic_inc(&ls->ls_stid.sc_file->fi_lo_recalls);
if (list_empty(&ls->ls_layouts))
goto out_unlock;
+ ls->ls_recalled = true;
+ atomic_inc(&ls->ls_stid.sc_file->fi_lo_recalls);
trace_nfsd_layout_recall(&ls->ls_stid.sc_stateid);
refcount_inc(&ls->ls_stid.sc_count);
--
2.31.1
> On Jan 27, 2023, at 11:18 AM, Benjamin Coddington <[email protected]> wrote:
>
> Its possible for __break_lease to find the layout's lease before we've
> added the layout to the owner's ls_layouts list. In that case, setting
> ls_recalled = true without actually recalling the layout will cause the
> server to never send a recall callback.
>
> Move the check for ls_layouts before setting ls_recalled.
>
> Signed-off-by: Benjamin Coddington <[email protected]>
Did this start misbehaving recently, or has it always been broken?
That is, does it need:
Fixes: c5c707f96fc9 ("nfsd: implement pNFS layout recalls") ?
> ---
> fs/nfsd/nfs4layouts.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
> index 3564d1c6f610..e8a80052cb1b 100644
> --- a/fs/nfsd/nfs4layouts.c
> +++ b/fs/nfsd/nfs4layouts.c
> @@ -323,11 +323,11 @@ nfsd4_recall_file_layout(struct nfs4_layout_stateid *ls)
> if (ls->ls_recalled)
> goto out_unlock;
>
> - ls->ls_recalled = true;
> - atomic_inc(&ls->ls_stid.sc_file->fi_lo_recalls);
> if (list_empty(&ls->ls_layouts))
> goto out_unlock;
>
> + ls->ls_recalled = true;
> + atomic_inc(&ls->ls_stid.sc_file->fi_lo_recalls);
> trace_nfsd_layout_recall(&ls->ls_stid.sc_stateid);
>
> refcount_inc(&ls->ls_stid.sc_count);
> --
> 2.31.1
>
--
Chuck Lever
On 27 Jan 2023, at 11:34, Chuck Lever III wrote:
>> On Jan 27, 2023, at 11:18 AM, Benjamin Coddington <[email protected]> wrote:
>>
>> Its possible for __break_lease to find the layout's lease before we've
>> added the layout to the owner's ls_layouts list. In that case, setting
>> ls_recalled = true without actually recalling the layout will cause the
>> server to never send a recall callback.
>>
>> Move the check for ls_layouts before setting ls_recalled.
>>
>> Signed-off-by: Benjamin Coddington <[email protected]>
>
> Did this start misbehaving recently, or has it always been broken?
> That is, does it need:
>
> Fixes: c5c707f96fc9 ("nfsd: implement pNFS layout recalls") ?
I'm doing some new testing of racing LAYOUTGET and CB_LAYOUTRETURN after
running into a livelock, so I think it has always been broken and the Fixes
tag is probably appropriate.
However, now I'm wondering if we'd run into trouble if ls_layouts could be
empty but the lease still exist.. but that seems like it would be a
different bug.
Ben
On Fri, 2023-01-27 at 11:42 -0500, Benjamin Coddington wrote:
> On 27 Jan 2023, at 11:34, Chuck Lever III wrote:
>
> > > On Jan 27, 2023, at 11:18 AM, Benjamin Coddington <[email protected]> wrote:
> > >
> > > Its possible for __break_lease to find the layout's lease before we've
> > > added the layout to the owner's ls_layouts list. In that case, setting
> > > ls_recalled = true without actually recalling the layout will cause the
> > > server to never send a recall callback.
> > >
> > > Move the check for ls_layouts before setting ls_recalled.
> > >
> > > Signed-off-by: Benjamin Coddington <[email protected]>
> >
> > Did this start misbehaving recently, or has it always been broken?
> > That is, does it need:
> >
> > Fixes: c5c707f96fc9 ("nfsd: implement pNFS layout recalls") ?
>
> I'm doing some new testing of racing LAYOUTGET and CB_LAYOUTRETURN after
> running into a livelock, so I think it has always been broken and the Fixes
> tag is probably appropriate.
>
> However, now I'm wondering if we'd run into trouble if ls_layouts could be
> empty but the lease still exist.. but that seems like it would be a
> different bug.
>
Yeah, is that even possible? Surely once the last layout is gone, we
drop the stateid? In any case, this patch looks fine. You can add:
Reviewed-by: Jeff Layton <[email protected]>
On 27 Jan 2023, at 13:03, Jeff Layton wrote:
> On Fri, 2023-01-27 at 11:42 -0500, Benjamin Coddington wrote:
>> On 27 Jan 2023, at 11:34, Chuck Lever III wrote:
>>
>>>> On Jan 27, 2023, at 11:18 AM, Benjamin Coddington <[email protected]> wrote:
>>>>
>>>> Its possible for __break_lease to find the layout's lease before we've
>>>> added the layout to the owner's ls_layouts list. In that case, setting
>>>> ls_recalled = true without actually recalling the layout will cause the
>>>> server to never send a recall callback.
>>>>
>>>> Move the check for ls_layouts before setting ls_recalled.
>>>>
>>>> Signed-off-by: Benjamin Coddington <[email protected]>
>>>
>>> Did this start misbehaving recently, or has it always been broken?
>>> That is, does it need:
>>>
>>> Fixes: c5c707f96fc9 ("nfsd: implement pNFS layout recalls") ?
>>
>> I'm doing some new testing of racing LAYOUTGET and CB_LAYOUTRETURN after
>> running into a livelock, so I think it has always been broken and the Fixes
>> tag is probably appropriate.
>>
>> However, now I'm wondering if we'd run into trouble if ls_layouts could be
>> empty but the lease still exist.. but that seems like it would be a
>> different bug.
>>
>
> Yeah, is that even possible? Surely once the last layout is gone, we
> drop the stateid? In any case, this patch looks fine. You can add:
>
> Reviewed-by: Jeff Layton <[email protected]>
Jeff pointed out that there's another problem here. We can't just skip
sending the callback if ls_layouts is empty, because then the process trying
to break the lease will end up spinning in __break_lease.
I think we can drop the list_empty() check altogether - it must be there so
that we don't race in and send a callback for a layout that's already been
returned, but I don't see any harm in that. Clients should just return
NO_MATCHING_LAYOUT.
Ben
On Sat, 2023-01-28 at 08:31 -0500, Benjamin Coddington wrote:
> On 27 Jan 2023, at 13:03, Jeff Layton wrote:
>
> > On Fri, 2023-01-27 at 11:42 -0500, Benjamin Coddington wrote:
> > > On 27 Jan 2023, at 11:34, Chuck Lever III wrote:
> > >
> > > > > On Jan 27, 2023, at 11:18 AM, Benjamin Coddington <[email protected]> wrote:
> > > > >
> > > > > Its possible for __break_lease to find the layout's lease before we've
> > > > > added the layout to the owner's ls_layouts list. In that case, setting
> > > > > ls_recalled = true without actually recalling the layout will cause the
> > > > > server to never send a recall callback.
> > > > >
> > > > > Move the check for ls_layouts before setting ls_recalled.
> > > > >
> > > > > Signed-off-by: Benjamin Coddington <[email protected]>
> > > >
> > > > Did this start misbehaving recently, or has it always been broken?
> > > > That is, does it need:
> > > >
> > > > Fixes: c5c707f96fc9 ("nfsd: implement pNFS layout recalls") ?
> > >
> > > I'm doing some new testing of racing LAYOUTGET and CB_LAYOUTRETURN after
> > > running into a livelock, so I think it has always been broken and the Fixes
> > > tag is probably appropriate.
> > >
> > > However, now I'm wondering if we'd run into trouble if ls_layouts could be
> > > empty but the lease still exist.. but that seems like it would be a
> > > different bug.
> > >
> >
> > Yeah, is that even possible? Surely once the last layout is gone, we
> > drop the stateid? In any case, this patch looks fine. You can add:
> >
> > Reviewed-by: Jeff Layton <[email protected]>
>
> Jeff pointed out that there's another problem here. We can't just skip
> sending the callback if ls_layouts is empty, because then the process trying
> to break the lease will end up spinning in __break_lease.
>
> I think we can drop the list_empty() check altogether - it must be there so
> that we don't race in and send a callback for a layout that's already been
> returned, but I don't see any harm in that. Clients should just return
> NO_MATCHING_LAYOUT.
>
The bigger worry (AFAICS) is that there is a potential race between
LAYOUTGET and CB_LAYOUTRECALL:
The lease is set very early in the LAYOUTGET process, and it can be
broken at any time beyond that point, even before LAYOUTGET is done and
has populated the ls_layouts list. If __break_lease gets called before
the list is populated, then the recall won't be sent (because ls_layouts
is still empty), but the LAYOUTGET will still complete successfully.
I think we need a check at the end of nfsd4_layoutget, after the
nfsd4_insert_layout call to see whether the lease has been broken. If it
has, then we should unwind everything and return NFS4ERR_RECALLCONFLICT.
--
Jeff Layton <[email protected]>
[ Cc'ing the original author of this code. ]
Proposed patch is here:
https://lore.kernel.org/linux-nfs/979eebe94ef380af6a5fdb831e78fd4c0946a59e.1674836262.git.bcodding@redhat.com/
> On Jan 28, 2023, at 8:47 AM, Jeff Layton <[email protected]> wrote:
>
> On Sat, 2023-01-28 at 08:31 -0500, Benjamin Coddington wrote:
>> On 27 Jan 2023, at 13:03, Jeff Layton wrote:
>>
>>> On Fri, 2023-01-27 at 11:42 -0500, Benjamin Coddington wrote:
>>>> On 27 Jan 2023, at 11:34, Chuck Lever III wrote:
>>>>
>>>>>> On Jan 27, 2023, at 11:18 AM, Benjamin Coddington <[email protected]> wrote:
>>>>>>
>>>>>> Its possible for __break_lease to find the layout's lease before we've
>>>>>> added the layout to the owner's ls_layouts list. In that case, setting
>>>>>> ls_recalled = true without actually recalling the layout will cause the
>>>>>> server to never send a recall callback.
>>>>>>
>>>>>> Move the check for ls_layouts before setting ls_recalled.
>>>>>>
>>>>>> Signed-off-by: Benjamin Coddington <[email protected]>
>>>>>
>>>>> Did this start misbehaving recently, or has it always been broken?
>>>>> That is, does it need:
>>>>>
>>>>> Fixes: c5c707f96fc9 ("nfsd: implement pNFS layout recalls") ?
>>>>
>>>> I'm doing some new testing of racing LAYOUTGET and CB_LAYOUTRETURN after
>>>> running into a livelock, so I think it has always been broken and the Fixes
>>>> tag is probably appropriate.
>>>>
>>>> However, now I'm wondering if we'd run into trouble if ls_layouts could be
>>>> empty but the lease still exist.. but that seems like it would be a
>>>> different bug.
>>>>
>>>
>>> Yeah, is that even possible? Surely once the last layout is gone, we
>>> drop the stateid? In any case, this patch looks fine. You can add:
>>>
>>> Reviewed-by: Jeff Layton <[email protected]>
>>
>> Jeff pointed out that there's another problem here. We can't just skip
>> sending the callback if ls_layouts is empty, because then the process trying
>> to break the lease will end up spinning in __break_lease.
>>
>> I think we can drop the list_empty() check altogether - it must be there so
>> that we don't race in and send a callback for a layout that's already been
>> returned, but I don't see any harm in that. Clients should just return
>> NO_MATCHING_LAYOUT.
>>
>
> The bigger worry (AFAICS) is that there is a potential race between
> LAYOUTGET and CB_LAYOUTRECALL:
>
> The lease is set very early in the LAYOUTGET process, and it can be
> broken at any time beyond that point, even before LAYOUTGET is done and
> has populated the ls_layouts list. If __break_lease gets called before
> the list is populated, then the recall won't be sent (because ls_layouts
> is still empty), but the LAYOUTGET will still complete successfully.
>
> I think we need a check at the end of nfsd4_layoutget, after the
> nfsd4_insert_layout call to see whether the lease has been broken. If it
> has, then we should unwind everything and return NFS4ERR_RECALLCONFLICT.
Shall I drop this fix from nfsd-next, then?
--
Chuck Lever
On Sat, 2023-01-28 at 14:15 +0000, Chuck Lever III wrote:
> [ Cc'ing the original author of this code. ]
>
> Proposed patch is here:
>
> https://lore.kernel.org/linux-nfs/979eebe94ef380af6a5fdb831e78fd4c0946a59e.1674836262.git.bcodding@redhat.com/
>
> > On Jan 28, 2023, at 8:47 AM, Jeff Layton <[email protected]> wrote:
> >
> > On Sat, 2023-01-28 at 08:31 -0500, Benjamin Coddington wrote:
> > > On 27 Jan 2023, at 13:03, Jeff Layton wrote:
> > >
> > > > On Fri, 2023-01-27 at 11:42 -0500, Benjamin Coddington wrote:
> > > > > On 27 Jan 2023, at 11:34, Chuck Lever III wrote:
> > > > >
> > > > > > > On Jan 27, 2023, at 11:18 AM, Benjamin Coddington <[email protected]> wrote:
> > > > > > >
> > > > > > > Its possible for __break_lease to find the layout's lease before we've
> > > > > > > added the layout to the owner's ls_layouts list. In that case, setting
> > > > > > > ls_recalled = true without actually recalling the layout will cause the
> > > > > > > server to never send a recall callback.
> > > > > > >
> > > > > > > Move the check for ls_layouts before setting ls_recalled.
> > > > > > >
> > > > > > > Signed-off-by: Benjamin Coddington <[email protected]>
> > > > > >
> > > > > > Did this start misbehaving recently, or has it always been broken?
> > > > > > That is, does it need:
> > > > > >
> > > > > > Fixes: c5c707f96fc9 ("nfsd: implement pNFS layout recalls") ?
> > > > >
> > > > > I'm doing some new testing of racing LAYOUTGET and CB_LAYOUTRETURN after
> > > > > running into a livelock, so I think it has always been broken and the Fixes
> > > > > tag is probably appropriate.
> > > > >
> > > > > However, now I'm wondering if we'd run into trouble if ls_layouts could be
> > > > > empty but the lease still exist.. but that seems like it would be a
> > > > > different bug.
> > > > >
> > > >
> > > > Yeah, is that even possible? Surely once the last layout is gone, we
> > > > drop the stateid? In any case, this patch looks fine. You can add:
> > > >
> > > > Reviewed-by: Jeff Layton <[email protected]>
> > >
> > > Jeff pointed out that there's another problem here. We can't just skip
> > > sending the callback if ls_layouts is empty, because then the process trying
> > > to break the lease will end up spinning in __break_lease.
> > >
> > > I think we can drop the list_empty() check altogether - it must be there so
> > > that we don't race in and send a callback for a layout that's already been
> > > returned, but I don't see any harm in that. Clients should just return
> > > NO_MATCHING_LAYOUT.
> > >
> >
> > The bigger worry (AFAICS) is that there is a potential race between
> > LAYOUTGET and CB_LAYOUTRECALL:
> >
> > The lease is set very early in the LAYOUTGET process, and it can be
> > broken at any time beyond that point, even before LAYOUTGET is done and
> > has populated the ls_layouts list. If __break_lease gets called before
> > the list is populated, then the recall won't be sent (because ls_layouts
> > is still empty), but the LAYOUTGET will still complete successfully.
> >
> > I think we need a check at the end of nfsd4_layoutget, after the
> > nfsd4_insert_layout call to see whether the lease has been broken. If it
> > has, then we should unwind everything and return NFS4ERR_RECALLCONFLICT.
>
> Shall I drop this fix from nfsd-next, then?
>
No, I think Ben's fix is still valid. The problem I'm seeing is a
different issue in the same area of the code. A follow-on patch to
address that is appropriate.
--
Jeff Layton <[email protected]>
> On Jan 28, 2023, at 10:20 AM, Jeff Layton <[email protected]> wrote:
>
> On Sat, 2023-01-28 at 14:15 +0000, Chuck Lever III wrote:
>> [ Cc'ing the original author of this code. ]
>>
>> Proposed patch is here:
>>
>> https://lore.kernel.org/linux-nfs/979eebe94ef380af6a5fdb831e78fd4c0946a59e.1674836262.git.bcodding@redhat.com/
>>
>>> On Jan 28, 2023, at 8:47 AM, Jeff Layton <[email protected]> wrote:
>>>
>>> On Sat, 2023-01-28 at 08:31 -0500, Benjamin Coddington wrote:
>>>> On 27 Jan 2023, at 13:03, Jeff Layton wrote:
>>>>
>>>>> On Fri, 2023-01-27 at 11:42 -0500, Benjamin Coddington wrote:
>>>>>> On 27 Jan 2023, at 11:34, Chuck Lever III wrote:
>>>>>>
>>>>>>>> On Jan 27, 2023, at 11:18 AM, Benjamin Coddington <[email protected]> wrote:
>>>>>>>>
>>>>>>>> Its possible for __break_lease to find the layout's lease before we've
>>>>>>>> added the layout to the owner's ls_layouts list. In that case, setting
>>>>>>>> ls_recalled = true without actually recalling the layout will cause the
>>>>>>>> server to never send a recall callback.
>>>>>>>>
>>>>>>>> Move the check for ls_layouts before setting ls_recalled.
>>>>>>>>
>>>>>>>> Signed-off-by: Benjamin Coddington <[email protected]>
>>>>>>>
>>>>>>> Did this start misbehaving recently, or has it always been broken?
>>>>>>> That is, does it need:
>>>>>>>
>>>>>>> Fixes: c5c707f96fc9 ("nfsd: implement pNFS layout recalls") ?
>>>>>>
>>>>>> I'm doing some new testing of racing LAYOUTGET and CB_LAYOUTRETURN after
>>>>>> running into a livelock, so I think it has always been broken and the Fixes
>>>>>> tag is probably appropriate.
>>>>>>
>>>>>> However, now I'm wondering if we'd run into trouble if ls_layouts could be
>>>>>> empty but the lease still exist.. but that seems like it would be a
>>>>>> different bug.
>>>>>>
>>>>>
>>>>> Yeah, is that even possible? Surely once the last layout is gone, we
>>>>> drop the stateid? In any case, this patch looks fine. You can add:
>>>>>
>>>>> Reviewed-by: Jeff Layton <[email protected]>
>>>>
>>>> Jeff pointed out that there's another problem here. We can't just skip
>>>> sending the callback if ls_layouts is empty, because then the process trying
>>>> to break the lease will end up spinning in __break_lease.
>>>>
>>>> I think we can drop the list_empty() check altogether - it must be there so
>>>> that we don't race in and send a callback for a layout that's already been
>>>> returned, but I don't see any harm in that. Clients should just return
>>>> NO_MATCHING_LAYOUT.
>>>>
>>>
>>> The bigger worry (AFAICS) is that there is a potential race between
>>> LAYOUTGET and CB_LAYOUTRECALL:
>>>
>>> The lease is set very early in the LAYOUTGET process, and it can be
>>> broken at any time beyond that point, even before LAYOUTGET is done and
>>> has populated the ls_layouts list. If __break_lease gets called before
>>> the list is populated, then the recall won't be sent (because ls_layouts
>>> is still empty), but the LAYOUTGET will still complete successfully.
>>>
>>> I think we need a check at the end of nfsd4_layoutget, after the
>>> nfsd4_insert_layout call to see whether the lease has been broken. If it
>>> has, then we should unwind everything and return NFS4ERR_RECALLCONFLICT.
>>
>> Shall I drop this fix from nfsd-next, then?
>>
>
> No, I think Ben's fix is still valid. The problem I'm seeing is a
> different issue in the same area of the code. A follow-on patch to
> address that is appropriate.
Thanks for clarifying... I wasn't sure whether y'all were planning
a replacement patch or an addendum. Sounds like the latter, so I'll
leave Ben's fix on the queue.
--
Chuck Lever