On Thu, 23 Jan 2025, Mike Snitzer wrote:
> On Fri, Dec 06, 2024 at 01:15:30PM +1100, NeilBrown wrote:
> > The flags NFS_LAYOUT_RETURN and NFS_LAYOUT_RETURN_LOCK are effectively
> > identical.
> > The only time either are cleared is in pnfs_clear_layoutreturn_waitbit(),
> > and there both are cleared.
> > The only time NFS_LAYOUT_RETURN is set is in pnfs_prepare_layoutreturn()
> > immediately after NFS_LAYOUT_RETURN_LOCK was set.
> > The only other time that NFS_LAYOUT_RETURN_LOCK is set is in
> > pnfs_mark_layout_stateid_invalid() if NFS_LAYOUT_RETURN was set but
> > NFS_LAYOUT_RETURN_LOCK was not set - but that is an impossible
> > combination given that else where the flags are set or cleared together.
> >
> > So we only need one of these flags. This patch discards
> > NFS_LAYOUT_RETURN_LOCK and does the test_and_set needed for exclusion with
> > NFS_LAYOUT_RETURN.
> >
> > Also the wake_up_bit in pnfs_clear_layoutreturn_waitbit() is changed to
> > clear_and_wake_up_bit() which includes all needed barriers internally.
> >
> > Signed-off-by: NeilBrown <neilb@xxxxxxx>
>
> I appreciate that you've done a general audit of the NFS code and
> looked to improve / optimize the wake_up_bit() callers, etc. But how
> did you test this specific patch's changes to the pnfs code?
mumble mumble mumble
>
> Reason I ask is if you look at the commit that introduced
> NFS_LAYOUT_RETURN_LOCK way back when:
> 6604b203fb63 pNFS: On error, do not send LAYOUTGET until the LAYOUTRETURN has completed
That commit was effectively reverted by
Commit: 61f454e30c18 ("pNFS: Fix a deadlock when coalescing writes and returning the layout")
I don't pretend to understand exactly what is going on with these flags
but I'm fairly confident that I didn't change behaviour that I didn't
intent to change.
And now that I found that commit I'm even more confident that
NFS_LAYOUT_RETURN_LOCK isn't needed.
Thanks for reviewing my code !!
NeilBrown
>
> You'll see that, with your patch, you've seem to have now reverted the
> code back to before stable@ commit 6604b203fb63 was applied.
>
> Now there may be merit to doing that due to other changes in the pnfs
> code that didn't exist back then but... your changes look suspicious
> given the evolution of this code.
>
> Mike
>
>
