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 <bcodding@xxxxxxxxxx> 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 <bcodding@xxxxxxxxxx>
> > > >
> > > > 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 <jlayton@xxxxxxxxxx>
>
> 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 <jlayton@xxxxxxxxxx>
