Re: [PATCH v4 04/17] xfs: Add xfs_dabuf defines

Allison Collins
2019-11-09T22:06:43Z

On 11/9/19 1:11 PM, Darrick J. Wong wrote:
> On Sat, Nov 09, 2019 at 10:32:17AM -0700, Allison Collins wrote:
>> On 11/8/19 12:19 PM, Darrick J. Wong wrote:
>>> On Wed, Nov 06, 2019 at 06:27:48PM -0700, Allison Collins wrote:
>>>> This patch adds two new defines XFS_DABUF_MAP_NOMAPPING and
>>>> XFS_DABUF_MAP_HOLE_OK. This helps to clean up hard numbers and
>>>> makes the code easier to read
>>>>
>>>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>>>> ---
>>>> fs/xfs/libxfs/xfs_attr.c | 14 +++++++-----
>>>> fs/xfs/libxfs/xfs_attr_leaf.c | 23 +++++++++++--------
>>>> fs/xfs/libxfs/xfs_attr_leaf.h | 3 +++
>>>> fs/xfs/libxfs/xfs_da_btree.c | 50 ++++++++++++++++++++++++++++--------------
>>>> fs/xfs/libxfs/xfs_dir2_block.c | 6 +++--
>>>> fs/xfs/libxfs/xfs_dir2_data.c | 3 ++-
>>>> fs/xfs/libxfs/xfs_dir2_leaf.c | 9 +++++---
>>>> fs/xfs/libxfs/xfs_dir2_node.c | 10 +++++----
>>>> fs/xfs/scrub/dabtree.c | 6 ++---
>>>> fs/xfs/scrub/dir.c | 4 +++-
>>>> fs/xfs/xfs_attr_inactive.c | 6 +++--
>>>> fs/xfs/xfs_attr_list.c | 16 +++++++++-----
>>>> 12 files changed, 97 insertions(+), 53 deletions(-)
>>>
>>> <snip>
>>>
>>>> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.h b/fs/xfs/libxfs/xfs_attr_leaf.h
>>>> index bb08800..017480e 100644
>>>> --- a/fs/xfs/libxfs/xfs_attr_leaf.h
>>>> +++ b/fs/xfs/libxfs/xfs_attr_leaf.h
>>>> @@ -16,6 +16,9 @@ struct xfs_da_state_blk;
>>>> struct xfs_inode;
>>>> struct xfs_trans;
>>>> +#define XFS_DABUF_MAP_NOMAPPING (-1) /* Caller doesn't have a mapping. */
>>>> +#define XFS_DABUF_MAP_HOLE_OK (-2) /* don't complain if we land in a hole. */
>>>
>>> These are parameters to xfs_da_{get,read,reada}_buf, please put them
>>> next to the declarations for those functions.
>> Hmm, I dont see that these are declared in a header file, but how about
>> fs/xfs/libxfs/xfs_da_btree.h ? Did I miss them somewhere?
>
> They should be declared around line 200 in that very file.
Ok, I found it... I foo-bared search command. Will update then. Thx!

Allison
>
> --D
>
>> Allison
>>
>>
>>>
>>> Also they probably ought to be explicitly cast to xfs_daddr_t, e.g.
>>>
>>> /* Force a fresh lookup for the dir/attr mapping. */
>>> #define XFS_DABUF_MAP_NOMAPPING ((xfs_daddr_t)-1)
>>>
>>> /* Don't complain if we land in a hole. */
>>> #define XFS_DABUF_MAP_HOLE_OK ((xfs_daddr_t)-2)
>>>
>>>> +
>>>> /*
>>>> * Used to keep a list of "remote value" extents when unlinking an inode.
>>>> */
>>>
>>> <snip>
>>>
>>>> diff --git a/fs/xfs/libxfs/xfs_dir2_block.c b/fs/xfs/libxfs/xfs_dir2_block.c
>>>> index 8dedc30..6bc7651 100644
>>>> --- a/fs/xfs/libxfs/xfs_dir2_block.c
>>>> +++ b/fs/xfs/libxfs/xfs_dir2_block.c
>>>> @@ -20,6 +20,7 @@
>>>> #include "xfs_error.h"
>>>> #include "xfs_trace.h"
>>>> #include "xfs_log.h"
>>>> +#include "xfs_attr_leaf.h"
>>>> /*
>>>> * Local function prototypes.
>>>> @@ -123,8 +124,9 @@ xfs_dir3_block_read(
>>>> struct xfs_mount *mp = dp->i_mount;
>>>> int err;
>>>> - err = xfs_da_read_buf(tp, dp, mp->m_dir_geo->datablk, -1, bpp,
>>>> - XFS_DATA_FORK, &xfs_dir3_block_buf_ops);
>>>> + err = xfs_da_read_buf(tp, dp, mp->m_dir_geo->datablk,
>>>> + XFS_DABUF_MAP_NOMAPPING, bpp, XFS_DATA_FORK,
>>>> + &xfs_dir3_block_buf_ops);
>>>> if (!err && tp && *bpp)
>>>> xfs_trans_buf_set_type(tp, *bpp, XFS_BLFT_DIR_BLOCK_BUF);
>>>> return err;
>>>
>>> I think this misses the xfs_dir3_data_read call in
>>> xfs_dir2_leaf_to_block?
>>>
>>>> diff --git a/fs/xfs/libxfs/xfs_dir2_data.c b/fs/xfs/libxfs/xfs_dir2_data.c
>>>> index 2c79be4..a4188de 100644
>>>> --- a/fs/xfs/libxfs/xfs_dir2_data.c
>>>> +++ b/fs/xfs/libxfs/xfs_dir2_data.c
>>>> @@ -17,6 +17,7 @@
>>>> #include "xfs_trans.h"
>>>> #include "xfs_buf_item.h"
>>>> #include "xfs_log.h"
>>>> +#include "xfs_attr_leaf.h"
>>>> static xfs_failaddr_t xfs_dir2_data_freefind_verify(
>>>> struct xfs_dir2_data_hdr *hdr, struct xfs_dir2_data_free *bf,
>>>> @@ -653,7 +654,7 @@ xfs_dir3_data_init(
>>>> * Get the buffer set up for the block.
>>>> */
>>>> error = xfs_da_get_buf(tp, dp, xfs_dir2_db_to_da(args->geo, blkno),
>>>> - -1, &bp, XFS_DATA_FORK);
>>>> + XFS_DABUF_MAP_NOMAPPING, &bp, XFS_DATA_FORK);
>>>> if (error)
>>>> return error;
>>>> bp->b_ops = &xfs_dir3_data_buf_ops;
>>>> diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
>>>> index b7046e2..a2cba6bd 100644
>>>> --- a/fs/xfs/libxfs/xfs_dir2_leaf.c
>>>> +++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
>>>> @@ -19,6 +19,7 @@
>>>> #include "xfs_trace.h"
>>>> #include "xfs_trans.h"
>>>> #include "xfs_buf_item.h"
>>>> +#include "xfs_attr_leaf.h"
>>>> /*
>>>> * Local function declarations.
>>>> @@ -311,7 +312,7 @@ xfs_dir3_leaf_get_buf(
>>>> bno < xfs_dir2_byte_to_db(args->geo, XFS_DIR2_FREE_OFFSET));
>>>> error = xfs_da_get_buf(tp, dp, xfs_dir2_db_to_da(args->geo, bno),
>>>> - -1, &bp, XFS_DATA_FORK);
>>>> + XFS_DABUF_MAP_NOMAPPING, &bp, XFS_DATA_FORK);
>>>> if (error)
>>>> return error;
>>>> @@ -594,7 +595,8 @@ xfs_dir2_leaf_addname(
>>>> trace_xfs_dir2_leaf_addname(args);
>>>> - error = xfs_dir3_leaf_read(tp, dp, args->geo->leafblk, -1, &lbp);
>>>> + error = xfs_dir3_leaf_read(tp, dp, args->geo->leafblk,
>>>> + XFS_DABUF_MAP_NOMAPPING, &lbp);
>>>> if (error)
>>>> return error;
>>>
>>> I think there are some missing conversions for xfs_dir3_data_read calls
>>> in xfs_dir2_leaf_addname...
>>>
>>>> @@ -1189,7 +1191,8 @@ xfs_dir2_leaf_lookup_int(
>>>> tp = args->trans;
>>>> mp = dp->i_mount;
>>>> - error = xfs_dir3_leaf_read(tp, dp, args->geo->leafblk, -1, &lbp);
>>>> + error = xfs_dir3_leaf_read(tp, dp, args->geo->leafblk,
>>>> + XFS_DABUF_MAP_NOMAPPING, &lbp);
>>>> if (error)
>>>> return error;
>>>
>>> ...and two more dir3_leaf_read calls further down in this function...
>>>
>>> ...and one more in xfs_dir2_leaf_trim_data...
>>>
>>>> diff --git a/fs/xfs/libxfs/xfs_dir2_node.c b/fs/xfs/libxfs/xfs_dir2_node.c
>>>> index 8bbd742..0a803e4 100644
>>>> --- a/fs/xfs/libxfs/xfs_dir2_node.c
>>>> +++ b/fs/xfs/libxfs/xfs_dir2_node.c
>>>> @@ -20,6 +20,7 @@
>>>> #include "xfs_trans.h"
>>>> #include "xfs_buf_item.h"
>>>> #include "xfs_log.h"
>>>> +#include "xfs_attr_leaf.h"
>>>> /*
>>>> * Function declarations.
>>>> @@ -227,7 +228,7 @@ xfs_dir2_free_read(
>>>> xfs_dablk_t fbno,
>>>> struct xfs_buf **bpp)
>>>> {
>>>> - return __xfs_dir3_free_read(tp, dp, fbno, -1, bpp);
>>>> + return __xfs_dir3_free_read(tp, dp, fbno, XFS_DABUF_MAP_NOMAPPING, bpp);
>>>> }
>>>> static int
>>>> @@ -237,7 +238,7 @@ xfs_dir2_free_try_read(
>>>> xfs_dablk_t fbno,
>>>> struct xfs_buf **bpp)
>>>> {
>>>> - return __xfs_dir3_free_read(tp, dp, fbno, -2, bpp);
>>>> + return __xfs_dir3_free_read(tp, dp, fbno, XFS_DABUF_MAP_HOLE_OK, bpp);
>>>> }
>>>> static int
>>>> @@ -254,7 +255,7 @@ xfs_dir3_free_get_buf(
>>>> struct xfs_dir3_icfree_hdr hdr;
>>>> error = xfs_da_get_buf(tp, dp, xfs_dir2_db_to_da(args->geo, fbno),
>>>> - -1, &bp, XFS_DATA_FORK);
>>>> + XFS_DABUF_MAP_NOMAPPING, &bp, XFS_DATA_FORK);
>>>> if (error)
>>>> return error;
>>>
>>> ...there's also a missing call in xfs_dir2_leafn_lookup_for_entry...
>>>
>>>> @@ -1495,7 +1496,8 @@ xfs_dir2_leafn_toosmall(
>>>> * Read the sibling leaf block.
>>>> */
>>>> error = xfs_dir3_leafn_read(state->args->trans, dp,
>>>> - blkno, -1, &bp);
>>>> + blkno, XFS_DABUF_MAP_NOMAPPING,
>>>> + &bp);
>>>> if (error)
>>>> return error;
>>>
>>> ...and another one in xfs_dir2_node_addname_int.
>>>
>>>> diff --git a/fs/xfs/scrub/dabtree.c b/fs/xfs/scrub/dabtree.c
>>>> index 77ff9f9..353455c 100644
>>>> --- a/fs/xfs/scrub/dabtree.c
>>>> +++ b/fs/xfs/scrub/dabtree.c
>>>> @@ -355,9 +355,9 @@ xchk_da_btree_block(
>>>> goto out_nobuf;
>>>> /* Read the buffer. */
>>>> - error = xfs_da_read_buf(dargs->trans, dargs->dp, blk->blkno, -2,
>>>> - &blk->bp, dargs->whichfork,
>>>> - &xchk_da_btree_buf_ops);
>>>> + error = xfs_da_read_buf(dargs->trans, dargs->dp, blk->blkno,
>>>> + XFS_DABUF_MAP_HOLE_OK, &blk->bp,
>>>> + dargs->whichfork, &xchk_da_btree_buf_ops);
>>>> if (!xchk_da_process_error(ds, level, &error))
>>>> goto out_nobuf;
>>>> if (blk->bp)
>>>> diff --git a/fs/xfs/scrub/dir.c b/fs/xfs/scrub/dir.c
>>>> index 1e2e117..eb0fa0f 100644
>>>> --- a/fs/xfs/scrub/dir.c
>>>> +++ b/fs/xfs/scrub/dir.c
>>>> @@ -18,6 +18,7 @@
>>>> #include "scrub/scrub.h"
>>>> #include "scrub/common.h"
>>>> #include "scrub/dabtree.h"
>>>> +#include "xfs_attr_leaf.h"
>>>> /* Set us up to scrub directories. */
>>>> int
>>>
>>> I also noticed missing conversions for xfs_dir3_data_read in
>>> xchk_dir_rec, xchk_directory_data_bestfree,
>>> xchk_directory_leaf1_bestfree, and xchk_directory_free_bestfree.
>>>
>>>> @@ -492,7 +493,8 @@ xchk_directory_leaf1_bestfree(
>>>> int error;
>>>> /* Read the free space block. */
>>>> - error = xfs_dir3_leaf_read(sc->tp, sc->ip, lblk, -1, &bp);
>>>> + error = xfs_dir3_leaf_read(sc->tp, sc->ip, lblk,
>>>> + XFS_DABUF_MAP_NOMAPPING, &bp);
>>>> if (!xchk_fblock_process_error(sc, XFS_DATA_FORK, lblk, &error))
>>>> goto out;
>>>> xchk_buffer_recheck(sc, bp);
>>>
>>> There's also a missing xfs_dir3_data_readahead conversion in
>>> xchk_parent_count_parent_dentries in fs/xfs/scrub/parent.c.
>>>
>>>> diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
>>>> index f83f11d..9c22915 100644
>>>> --- a/fs/xfs/xfs_attr_inactive.c
>>>> +++ b/fs/xfs/xfs_attr_inactive.c
>>>> @@ -235,7 +235,8 @@ xfs_attr3_node_inactive(
>>>> * traversal of the tree so we may deal with many blocks
>>>> * before we come back to this one.
>>>> */
>>>> - error = xfs_da3_node_read(*trans, dp, child_fsb, -1, &child_bp,
>>>> + error = xfs_da3_node_read(*trans, dp, child_fsb,
>>>> + XFS_DABUF_MAP_NOMAPPING, &child_bp,
>>>> XFS_ATTR_FORK);
>>>> if (error)
>>>> return error;
>>>> @@ -321,7 +322,8 @@ xfs_attr3_root_inactive(
>>>> * the extents in reverse order the extent containing
>>>> * block 0 must still be there.
>>>> */
>>>> - error = xfs_da3_node_read(*trans, dp, 0, -1, &bp, XFS_ATTR_FORK);
>>>> + error = xfs_da3_node_read(*trans, dp, 0, XFS_DABUF_MAP_NOMAPPING, &bp,
>>>> + XFS_ATTR_FORK);
>>>> if (error)
>>>> return error;
>>>> blkno = bp->b_bn;
>>>> diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
>>>> index c02f22d..fab416c 100644
>>>> --- a/fs/xfs/xfs_attr_list.c
>>>> +++ b/fs/xfs/xfs_attr_list.c
>>>> @@ -224,8 +224,9 @@ xfs_attr_node_list_lookup(
>>>> ASSERT(*pbp == NULL);
>>>> cursor->blkno = 0;
>>>> for (;;) {
>>>> - error = xfs_da3_node_read(tp, dp, cursor->blkno, -1, &bp,
>>>> - XFS_ATTR_FORK);
>>>> + error = xfs_da3_node_read(tp, dp, cursor->blkno,
>>>> + XFS_DABUF_MAP_NOMAPPING, &bp,
>>>> + XFS_ATTR_FORK);
>>>> if (error)
>>>> return error;
>>>> node = bp->b_addr;
>>>> @@ -309,8 +310,9 @@ xfs_attr_node_list(
>>>> */
>>>> bp = NULL;
>>>> if (cursor->blkno > 0) {
>>>> - error = xfs_da3_node_read(context->tp, dp, cursor->blkno, -1,
>>>> - &bp, XFS_ATTR_FORK);
>>>> + error = xfs_da3_node_read(context->tp, dp, cursor->blkno,
>>>> + XFS_DABUF_MAP_NOMAPPING, &bp,
>>>> + XFS_ATTR_FORK);
>>>> if ((error != 0) && (error != -EFSCORRUPTED))
>>>> return error;
>>>> if (bp) {
>>>> @@ -377,7 +379,8 @@ xfs_attr_node_list(
>>>> break;
>>>> cursor->blkno = leafhdr.forw;
>>>> xfs_trans_brelse(context->tp, bp);
>>>> - error = xfs_attr3_leaf_read(context->tp, dp, cursor->blkno, -1, &bp);
>>>> + error = xfs_attr3_leaf_read(context->tp, dp, cursor->blkno,
>>>> + XFS_DABUF_MAP_NOMAPPING, &bp);
>>>> if (error)
>>>> return error;
>>>> }
>>>> @@ -497,7 +500,8 @@ xfs_attr_leaf_list(xfs_attr_list_context_t *context)
>>>> trace_xfs_attr_leaf_list(context);
>>>> context->cursor->blkno = 0;
>>>> - error = xfs_attr3_leaf_read(context->tp, context->dp, 0, -1, &bp);
>>>> + error = xfs_attr3_leaf_read(context->tp, context->dp, 0,
>>>> + XFS_DABUF_MAP_NOMAPPING, &bp);
>>>> if (error)
>>>> return error;
>>>
>>> ...more missing conversions of xfs_dir3_data_read in
>>> xfs_dir2_leaf_readbuf; and of xfs_dir3_data_readahead in
>>> xfs_dir2_leaf_readbuf...
>>>
>>> ...and a missing conversion of xfs_dir3_data_readahead in xfs_dir_open
>>> in fs/xfs/xfs_file.c.
>>>
>>> --D
>>>
>>>> --
>>>> 2.7.4
>>>>