On 8/28/23 04:44, Matthew Wilcox wrote:
On Sun, Aug 27, 2023 at 09:28:26PM +0800, Hao Xu wrote:
+++ b/fs/xfs/libxfs/xfs_da_btree.c
@@ -2643,16 +2643,32 @@ xfs_da_read_buf(
struct xfs_buf_map map, *mapp = ↦
int nmap = 1;
int error;
+ int buf_flags = 0;
*bpp = NULL;
error = xfs_dabuf_map(dp, bno, flags, whichfork, &mapp, &nmap);
if (error || !nmap)
goto out_free;
+ /*
+ * NOWAIT semantics mean we don't wait on the buffer lock nor do we
+ * issue IO for this buffer if it is not already in memory. Caller will
+ * retry. This will return -EAGAIN if the buffer is in memory and cannot
+ * be locked, and no buffer and no error if it isn't in memory. We
+ * translate both of those into a return state of -EAGAIN and *bpp =
+ * NULL.
+ */
I would not include this comment.
No strong comment here, since this patch is mostly from Dave, it's
better if Dave can ack this.
+ if (flags & XFS_DABUF_NOWAIT)
+ buf_flags |= XBF_TRYLOCK | XBF_INCORE;
error = xfs_trans_read_buf_map(mp, tp, mp->m_ddev_targp, mapp, nmap, 0,
&bp, ops);
what tsting did you do with this? Because you don't actually _use_
buf_flags anywhere in this patch (presumably they should be the
sixth argument to xfs_trans_read_buf_map() instead of 0). So I can only
conclude that either you didn't test, or your testing was inadequate.
The tests I've done are listed in the cover-letter, this one is missed,
the tricky place is it's hard to get this kind of mistake since it runs
well without nowait logic...I'll fix it in next version.
if (error)
goto out_free;
+ if (!bp) {
+ ASSERT(flags & XFS_DABUF_NOWAIT);
I don't think this ASSERT is appropriate.
@@ -391,10 +401,17 @@ xfs_dir2_leaf_getdents(
bp = NULL;
}
- if (*lock_mode == 0)
- *lock_mode = xfs_ilock_data_map_shared(dp);
+ if (*lock_mode == 0) {
+ *lock_mode =
+ xfs_ilock_data_map_shared_generic(dp,
+ ctx->flags & DIR_CONTEXT_F_NOWAIT);
+ if (!*lock_mode) {
+ error = -EAGAIN;
+ break;
+ }
+ }
'generic' doesn't seem like a great suffix to mean 'takes nowait flag'.
And this is far too far indented.
xfs_dir2_lock(dp, ctx, lock_mode);
with:
STATIC void xfs_dir2_lock(struct xfs_inode *dp, struct dir_context *ctx,
unsigned int lock_mode)
{
if (*lock_mode)
return;
if (ctx->flags & DIR_CONTEXT_F_NOWAIT)
return xfs_ilock_data_map_shared_nowait(dp);
return xfs_ilock_data_map_shared(dp);
}
... which I think you can use elsewhere in this patch (reformat it to
XFS coding style, of course). And then you don't need
xfs_ilock_data_map_shared_generic().
How about rename xfs_ilock_data_map_shared() to
xfs_ilock_data_map_block() and rename
xfs_ilock_data_map_shared_generic() to xfs_ilock_data_map_shared()?
STATIC void xfs_ilock_data_map_shared(struct xfs_inode *dp, struct
dir_context *ctx, unsigned int lock_mode)
{
if (*lock_mode)
return;
if (ctx->flags & DIR_CONTEXT_F_NOWAIT)
return xfs_ilock_data_map_shared_nowait(dp);
return xfs_ilock_data_map_shared_block(dp);
}
--
Linux-cachefs mailing list
Linux-cachefs@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/linux-cachefs
