From:   Benjamin Coddington <bcodding@redhat.com>
To:     Jeff Layton <jlayton@kernel.org>
Cc:     Chuck Lever III <chuck.lever@oracle.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH] nfsd: fix race to check ls_layouts
Date:   Sat, 28 Jan 2023 08:31:58 -0500
Message-ID: <A1324B12-C0FE-4A60-8116-4DCD98C92A8D@redhat.com>
In-Reply-To: <0412daf06541dfa71866be1efedef5456e524ece.camel@kernel.org>
References: <979eebe94ef380af6a5fdb831e78fd4c0946a59e.1674836262.git.bcodding@redhat.com>
 <C9FA580A-52A6-4208-AFA2-91E8BCB36B9F@oracle.com>
 <49815925-FB60-456F-8633-79B4C203B782@redhat.com>
 <0412daf06541dfa71866be1efedef5456e524ece.camel@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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@redhat.com> 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@redhat.com>
>>>
>>> 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@kernel.org>

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