From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Allison Collins <allison.henderson@oracle.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH v4 04/17] xfs: Add xfs_dabuf defines
Date: Sat, 9 Nov 2019 12:11:46 -0800 [thread overview]
Message-ID: <20191109201146.GO6219@magnolia> (raw)
In-Reply-To: <a8db403f-f996-7352-0845-079630e1dae5@oracle.com>
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.
--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
> > >
next prev parent reply other threads:[~2019-11-09 20:11 UTC|newest]
Thread overview: 77+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-07 1:27 [PATCH v4 00/17] xfs: Delay Ready Attributes Allison Collins
2019-11-07 1:27 ` [PATCH v4 01/17] xfs: Remove all strlen in all xfs_attr_* functions for attr names Allison Collins
2019-11-11 17:47 ` Christoph Hellwig
2019-11-11 23:35 ` Allison Collins
2019-11-07 1:27 ` [PATCH v4 02/17] xfs: Replace attribute parameters with struct xfs_name Allison Collins
2019-11-08 1:13 ` Darrick J. Wong
2019-11-08 17:16 ` Allison Collins
2019-11-11 17:49 ` Christoph Hellwig
2019-11-11 20:07 ` Allison Collins
2019-11-13 15:12 ` Allison Collins
2019-11-20 18:20 ` Christoph Hellwig
2019-11-07 1:27 ` [PATCH v4 03/17] xfs: Embed struct xfs_name in xfs_da_args Allison Collins
2019-11-08 1:25 ` Darrick J. Wong
2019-11-08 16:11 ` Allison Collins
2019-11-08 21:47 ` Darrick J. Wong
2019-11-07 1:27 ` [PATCH v4 04/17] xfs: Add xfs_dabuf defines Allison Collins
2019-11-08 19:19 ` Darrick J. Wong
2019-11-09 17:32 ` Allison Collins
2019-11-09 20:11 ` Darrick J. Wong [this message]
2019-11-09 22:06 ` Allison Collins
2019-11-07 1:27 ` [PATCH v4 05/17] xfs: Add xfs_has_attr and subroutines Allison Collins
2019-11-08 19:32 ` Darrick J. Wong
2019-11-08 19:51 ` Allison Collins
2019-11-11 17:40 ` Brian Foster
2019-11-11 23:34 ` Allison Collins
2019-11-11 17:53 ` Christoph Hellwig
2019-11-11 23:36 ` Allison Collins
2019-11-07 1:27 ` [PATCH v4 06/17] xfs: Factor out new helper functions xfs_attr_rmtval_set Allison Collins
2019-11-08 19:34 ` Darrick J. Wong
2019-11-08 19:51 ` Allison Collins
2019-11-07 1:27 ` [PATCH v4 07/17] xfs: Factor up trans handling in xfs_attr3_leaf_flipflags Allison Collins
2019-11-08 19:35 ` Darrick J. Wong
2019-11-08 19:52 ` Allison Collins
2019-11-07 1:27 ` [PATCH v4 08/17] xfs: Factor out xfs_attr_leaf_addname helper Allison Collins
2019-11-08 20:57 ` Darrick J. Wong
2019-11-09 21:41 ` Allison Collins
2019-11-07 1:27 ` [PATCH v4 09/17] xfs: Factor up commit from xfs_attr_try_sf_addname Allison Collins
2019-11-08 21:04 ` Darrick J. Wong
2019-11-08 23:13 ` Allison Collins
2019-11-07 1:27 ` [PATCH v4 10/17] xfs: Factor up trans roll from xfs_attr3_leaf_setflag Allison Collins
2019-11-07 1:27 ` [PATCH v4 11/17] xfs: Add xfs_attr3_leaf helper functions Allison Collins
2019-11-08 21:17 ` Darrick J. Wong
2019-11-09 0:09 ` Allison Collins
2019-11-07 1:27 ` [PATCH v4 12/17] xfs: Factor out xfs_attr_rmtval_invalidate Allison Collins
2019-11-08 21:19 ` Darrick J. Wong
2019-11-09 0:10 ` Allison Collins
2019-11-07 1:27 ` [PATCH v4 13/17] xfs: Factor up trans roll in xfs_attr3_leaf_clearflag Allison Collins
2019-11-08 21:19 ` Darrick J. Wong
2019-11-09 0:11 ` Allison Collins
2019-11-11 18:23 ` Brian Foster
2019-11-11 23:37 ` Allison Collins
2019-11-07 1:27 ` [PATCH v4 14/17] xfs: Add delay context to xfs_da_args Allison Collins
2019-11-08 21:22 ` Darrick J. Wong
2019-11-09 0:23 ` Allison Collins
2019-11-11 18:23 ` Brian Foster
2019-11-11 23:42 ` Allison Collins
2019-11-07 1:27 ` [PATCH v4 15/17] xfs: Check for -ENOATTR or -EEXIST Allison Collins
2019-11-08 21:28 ` Darrick J. Wong
2019-11-08 21:42 ` Allison Collins
2019-11-08 21:51 ` Darrick J. Wong
2019-11-11 18:24 ` Brian Foster
2019-11-12 0:33 ` Allison Collins
2019-11-07 1:28 ` [PATCH v4 16/17] xfs: Add delay ready attr remove routines Allison Collins
2019-11-08 21:37 ` Darrick J. Wong
2019-11-09 0:25 ` Allison Collins
2019-11-12 13:37 ` Brian Foster
2019-11-13 0:43 ` Allison Collins
2019-11-13 11:54 ` Brian Foster
2019-11-13 23:39 ` Allison Collins
2019-11-14 12:48 ` Brian Foster
2019-11-14 17:58 ` Allison Collins
2019-11-07 1:28 ` [PATCH v4 17/17] xfs: Add delay ready attr set routines Allison Collins
2019-11-08 21:42 ` Darrick J. Wong
2019-11-08 21:52 ` Allison Collins
2019-11-09 4:07 ` Allison Collins
2019-11-12 13:37 ` Brian Foster
2019-11-13 4:57 ` Allison Collins
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191109201146.GO6219@magnolia \
--to=darrick.wong@oracle.com \
--cc=allison.henderson@oracle.com \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox