Re: [PATCH 24/29] xfs: support nowait for xfs_buf_read_map()

Dave Chinner
2023-08-25T21:54:58Z

On Fri, Aug 25, 2023 at 09:54:26PM +0800, Hao Xu wrote:
> From: Hao Xu <howeyxu@tencent.com>
>
> This causes xfstests generic/232 hung in umount process, waiting for ail
> push, so I comment it for now, need some hints from xfs folks.
> Not a real patch.
>
> Signed-off-by: Hao Xu <howeyxu@tencent.com>
> ---
> fs/xfs/xfs_buf.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index cdad80e1ae25..284962a9f31a 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -828,6 +828,13 @@ xfs_buf_read_map(
> trace_xfs_buf_read(bp, flags, _RET_IP_);
>
> if (!(bp->b_flags & XBF_DONE)) {
> +// /*
> +// * Let's bypass the _xfs_buf_read() for now
> +// */
> +// if (flags & XBF_NOWAIT) {
> +// xfs_buf_relse(bp);
> +// return -EAGAIN;
> +// }

This is *fundamentally broken*, and apart from anything else breaks
readahead.

IF we asked for a read, we cannot instantiate the buffer and then
*not issue any IO on it* and release it. That leaves an
uninitialised buffer in memory, and there's every chance that
something then trips over it and bad things happen.

A buffer like this *must* be errored out and marked stale so that
the next access to it will then re-initialise the buffer state and
trigger any preparatory work that needs to be done for the new
operation.

This comes back to my first comments that XBF_TRYLOCK cannot simpy
be replaced with XBF_NOWAIT semantics...

-Dave.
--
Dave Chinner
david@fromorbit.com