From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Allison Henderson <allison.henderson@oracle.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [RFC PATCH 02/13] xfs: get directory offset when removing directory name
Date: Mon, 14 Aug 2017 16:56:53 -0700 [thread overview]
Message-ID: <20170814235653.GQ4796@magnolia> (raw)
In-Reply-To: <1502329293-4091-3-git-send-email-allison.henderson@oracle.com>
On Wed, Aug 09, 2017 at 06:41:22PM -0700, Allison Henderson wrote:
> From: Mark Tinguely <tinguely@sgi.com>
>
> Return the directory offset information when removing an entry to the
> directory.
>
> This offset will be used as the parent pointer offset in xfs_remove.
>
> [dchinner: forward ported and cleaned up]
> [achender: rebased, changed __unint32_t to xfs_dir2_dataptr_t]
>
> Signed-off-by: Mark Tinguely <tinguely@sgi.com>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> ---
> :100644 100644 a1ca460... a34956e... M fs/xfs/libxfs/xfs_dir2.c
> :100644 100644 e349900... ee98b2a... M fs/xfs/libxfs/xfs_dir2.h
> :100644 100644 79684d5... 4dbe2fc... M fs/xfs/libxfs/xfs_dir2_block.c
> :100644 100644 2ac7a7e... 197e627... M fs/xfs/libxfs/xfs_dir2_leaf.c
> :100644 100644 8bc91f8... 13d5244... M fs/xfs/libxfs/xfs_dir2_node.c
> :100644 100644 489bdef... 9e90c22... M fs/xfs/libxfs/xfs_dir2_sf.c
> :100644 100644 cea1c56... f58f62a... M fs/xfs/xfs_inode.c
> fs/xfs/libxfs/xfs_dir2.c | 15 +++++++++------
> fs/xfs/libxfs/xfs_dir2.h | 3 ++-
> fs/xfs/libxfs/xfs_dir2_block.c | 4 ++--
> fs/xfs/libxfs/xfs_dir2_leaf.c | 5 +++--
> fs/xfs/libxfs/xfs_dir2_node.c | 5 +++--
> fs/xfs/libxfs/xfs_dir2_sf.c | 2 ++
> fs/xfs/xfs_inode.c | 7 ++++---
> 7 files changed, 25 insertions(+), 16 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
> index a1ca460..a34956e 100644
> --- a/fs/xfs/libxfs/xfs_dir2.c
> +++ b/fs/xfs/libxfs/xfs_dir2.c
> @@ -443,13 +443,14 @@ xfs_dir_lookup(
> */
> int
> xfs_dir_removename(
> - xfs_trans_t *tp,
> - xfs_inode_t *dp,
> - struct xfs_name *name,
> - xfs_ino_t ino,
> - xfs_fsblock_t *first, /* bmap's firstblock */
> + xfs_trans_t *tp,
If you change a line containing a struct tyepdef type, please convert it
back to the raw struct type.
struct xfs_trans *tp,
etc. Some day we'll have managed to remove them all! :D
> + xfs_inode_t *dp,
> + struct xfs_name *name,
> + xfs_ino_t ino,
> + xfs_fsblock_t *first, /* bmap's firstblock */
> struct xfs_defer_ops *dfops, /* bmap's freeblock list */
> - xfs_extlen_t total) /* bmap's total block count */
> + xfs_extlen_t total, /* bmap's total block count */
> + xfs_dir2_dataptr_t *offset) /* OUT: offset in directory */
Hmm. We /also/ pass a defer_ops into _dir_removename. I wasn't going
to say this until later in the patchset, but it's apropos now.
The biggest complaint I have about this patch series is that we end up
using separate transactions for each phase of the link/unlink
operations. There's no guarantee that once we start the process of
expand-dir, add-name-to-dir, log-inode-cores, expand-attr,
add-pptr-to-attrs that we actually get all the way to the end of that
process, particularly if we crash in the middle and have to recover the
log coming back up.
Obviously, back in 2014 when Dave started on these patches there was no
defer_ops, so a lot of effort goes into passing parameters up and down
the stack and fiddling with the transaction reservation type
declarations in order to guarantee that we can shove everything into a
single transaction. Now that we have a more general mechanism to split
complex updates into a chain of smaller updates (and make log recovery
pick up where we left off) it's time to figure out how to make attr (and
maybe dir) updates a deferrable operation.
This adds complexity to the log because we have to define an "attr add
KEY:VALUE" and a "attr delete KEY" deferred op that hooks into the
existing attr code. For parent pointers it's not a big deal to log
values since they're not big, but for the general case this idea needs
more thought, or a decision that we'll just keep doing things the same
way.
Anyway, assuming the existence of a "attr delete KEY" log intent item we
no longer have to pass the offsets around everywhere because instead
_dir_removename attaches the defer item to the defer_ops and
_defer_finish takes care of rolling through the updates.
Thoughts?
--D
> {
> struct xfs_da_args *args;
> int rval;
> @@ -495,6 +496,8 @@ xfs_dir_removename(
> rval = xfs_dir2_leaf_removename(args);
> else
> rval = xfs_dir2_node_removename(args);
> + if (offset)
> + *offset = args->offset;
> out_free:
> kmem_free(args);
> return rval;
> diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
> index e349900..ee98b2a 100644
> --- a/fs/xfs/libxfs/xfs_dir2.h
> +++ b/fs/xfs/libxfs/xfs_dir2.h
> @@ -139,7 +139,8 @@ extern int xfs_dir_lookup(struct xfs_trans *tp, struct xfs_inode *dp,
> extern int xfs_dir_removename(struct xfs_trans *tp, struct xfs_inode *dp,
> struct xfs_name *name, xfs_ino_t ino,
> xfs_fsblock_t *first,
> - struct xfs_defer_ops *dfops, xfs_extlen_t tot);
> + struct xfs_defer_ops *dfops, xfs_extlen_t tot,
> + xfs_dir2_dataptr_t *offset);
> extern int xfs_dir_replace(struct xfs_trans *tp, struct xfs_inode *dp,
> struct xfs_name *name, xfs_ino_t inum,
> xfs_fsblock_t *first,
> diff --git a/fs/xfs/libxfs/xfs_dir2_block.c b/fs/xfs/libxfs/xfs_dir2_block.c
> index 79684d5..4dbe2fc 100644
> --- a/fs/xfs/libxfs/xfs_dir2_block.c
> +++ b/fs/xfs/libxfs/xfs_dir2_block.c
> @@ -791,9 +791,9 @@ xfs_dir2_block_removename(
> /*
> * Point to the data entry using the leaf entry.
> */
> + args->offset = be32_to_cpu(blp[ent].address);
> dep = (xfs_dir2_data_entry_t *)((char *)hdr +
> - xfs_dir2_dataptr_to_off(args->geo,
> - be32_to_cpu(blp[ent].address)));
> + xfs_dir2_dataptr_to_off(args->geo, args->offset));
> /*
> * Mark the data entry's space free.
> */
> diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
> index 2ac7a7e..197e627 100644
> --- a/fs/xfs/libxfs/xfs_dir2_leaf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
> @@ -1383,9 +1383,10 @@ xfs_dir2_leaf_removename(
> * Point to the leaf entry, use that to point to the data entry.
> */
> lep = &ents[index];
> - db = xfs_dir2_dataptr_to_db(args->geo, be32_to_cpu(lep->address));
> + args->offset = be32_to_cpu(lep->address);
> + db = xfs_dir2_dataptr_to_db(args->geo, args->offset);
> dep = (xfs_dir2_data_entry_t *)((char *)hdr +
> - xfs_dir2_dataptr_to_off(args->geo, be32_to_cpu(lep->address)));
> + xfs_dir2_dataptr_to_off(args->geo, args->offset));
> needscan = needlog = 0;
> oldbest = be16_to_cpu(bf[0].length);
> ltp = xfs_dir2_leaf_tail_p(args->geo, leaf);
> diff --git a/fs/xfs/libxfs/xfs_dir2_node.c b/fs/xfs/libxfs/xfs_dir2_node.c
> index 8bc91f8..13d5244 100644
> --- a/fs/xfs/libxfs/xfs_dir2_node.c
> +++ b/fs/xfs/libxfs/xfs_dir2_node.c
> @@ -1238,9 +1238,10 @@ xfs_dir2_leafn_remove(
> /*
> * Extract the data block and offset from the entry.
> */
> - db = xfs_dir2_dataptr_to_db(args->geo, be32_to_cpu(lep->address));
> + args->offset = be32_to_cpu(lep->address);
> + db = xfs_dir2_dataptr_to_db(args->geo, args->offset);
> ASSERT(dblk->blkno == db);
> - off = xfs_dir2_dataptr_to_off(args->geo, be32_to_cpu(lep->address));
> + off = xfs_dir2_dataptr_to_off(args->geo, args->offset);
> ASSERT(dblk->index == off);
>
> /*
> diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
> index 489bdef..9e90c22 100644
> --- a/fs/xfs/libxfs/xfs_dir2_sf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_sf.c
> @@ -919,6 +919,8 @@ xfs_dir2_sf_removename(
> XFS_CMP_EXACT) {
> ASSERT(dp->d_ops->sf_get_ino(sfp, sfep) ==
> args->inumber);
> + args->offset = xfs_dir2_byte_to_dataptr(
> + xfs_dir2_sf_get_offset(sfep));
> break;
> }
> }
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index cea1c56..f58f62a 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -2621,8 +2621,8 @@ xfs_remove(
> goto out_trans_cancel;
>
> xfs_defer_init(&dfops, &first_block);
> - error = xfs_dir_removename(tp, dp, name, ip->i_ino,
> - &first_block, &dfops, resblks);
> + error = xfs_dir_removename(tp, dp, name, ip->i_ino, &first_block,
> + &dfops, resblks, NULL);
> if (error) {
> ASSERT(error != -ENOENT);
> goto out_bmap_cancel;
> @@ -3132,7 +3132,8 @@ xfs_rename(
> &first_block, &dfops, spaceres);
> } else
> error = xfs_dir_removename(tp, src_dp, src_name, src_ip->i_ino,
> - &first_block, &dfops, spaceres);
> + &first_block, &dfops, spaceres,
> + NULL);
> if (error)
> goto out_bmap_cancel;
>
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2017-08-14 23:56 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-10 1:41 [RFC PATCH 00/13] Add parent pointer attributes Allison Henderson
2017-08-10 1:41 ` [RFC PATCH 01/13] xfs: get directory offset when adding directory name Allison Henderson
2017-08-10 1:41 ` [RFC PATCH 02/13] xfs: get directory offset when removing " Allison Henderson
2017-08-14 23:56 ` Darrick J. Wong [this message]
2017-08-15 3:23 ` Dave Chinner
2017-08-15 5:47 ` Dave Chinner
2017-08-15 3:54 ` Allison Henderson
2017-08-10 1:41 ` [RFC PATCH 03/13] xfs: get directory offset when replacing a " Allison Henderson
2017-08-10 1:41 ` [RFC PATCH 04/13] xfs: add parent pointer support to attribute code Allison Henderson
2017-08-10 1:41 ` [RFC PATCH 05/13] xfs: define parent pointer xattr format Allison Henderson
2017-08-14 23:59 ` Darrick J. Wong
2017-08-15 4:05 ` Allison Henderson
2017-08-10 1:41 ` [RFC PATCH 06/13] :xfs: extent transaction reservations for parent attributes Allison Henderson
2017-08-10 1:41 ` [RFC PATCH 07/13] Add the extra space requirements for parent pointer attributes when calculating the minimum log size during mkfs Allison Henderson
2017-08-10 1:41 ` [RFC PATCH 08/13] xfs: parent pointer attribute creation Allison Henderson
2017-08-15 0:06 ` Darrick J. Wong
2017-08-15 3:32 ` Dave Chinner
2017-08-15 4:09 ` Allison Henderson
2017-08-10 1:41 ` [RFC PATCH 09/13] xfs: add parent attributes to link Allison Henderson
2017-08-10 1:41 ` [RFC PATCH 10/13] xfs: remove parent pointers in unlink Allison Henderson
2017-08-10 1:41 ` [RFC PATCH 11/13] xfs_bmap_add_attrfork(): re-add error handling from set_attrforkoff() call Allison Henderson
2017-08-10 1:41 ` [RFC PATCH 12/13] Add parent pointers to rename Allison Henderson
2017-08-10 1:41 ` [RFC PATCH 13/13] Add the parent pointer support to the superblock version 5 Allison Henderson
2017-08-15 0:07 ` Darrick J. Wong
2017-08-15 4:11 ` Allison Henderson
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=20170814235653.GQ4796@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