Re: [PATCH v24 03/11] xfs: Set up infrastructure for log atrribute replay

[Dave Chinner <david@fromorbit.com>]

On Tue, Aug 24, 2021 at 03:44:26PM -0700, Allison Henderson wrote:
> Currently attributes are modified directly across one or more
> transactions. But they are not logged or replayed in the event of an
> error. The goal of log attr replay is to enable logging and replaying
> of attribute operations using the existing delayed operations
> infrastructure.  This will later enable the attributes to become part of
> larger multi part operations that also must first be recorded to the
> log.  This is mostly of interest in the scheme of parent pointers which
> would need to maintain an attribute containing parent inode information
> any time an inode is moved, created, or removed.  Parent pointers would
> then be of interest to any feature that would need to quickly derive an
> inode path from the mount point. Online scrub, nfs lookups and fs grow
> or shrink operations are all features that could take advantage of this.
> 
> This patch adds two new log item types for setting or removing
> attributes as deferred operations.  The xfs_attri_log_item will log an
> intent to set or remove an attribute.  The corresponding
> xfs_attrd_log_item holds a reference to the xfs_attri_log_item and is
> freed once the transaction is done.  Both log items use a generic
> xfs_attr_log_format structure that contains the attribute name, value,
> flags, inode, and an op_flag that indicates if the operations is a set
> or remove.
> 
> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>  fs/xfs/Makefile                 |   1 +
>  fs/xfs/libxfs/xfs_attr.c        |   5 +-
>  fs/xfs/libxfs/xfs_attr.h        |  31 +++
>  fs/xfs/libxfs/xfs_defer.h       |   2 +
>  fs/xfs/libxfs/xfs_log_format.h  |  44 +++-
>  fs/xfs/libxfs/xfs_log_recover.h |   2 +
>  fs/xfs/scrub/common.c           |   2 +
>  fs/xfs/xfs_attr_item.c          | 453 ++++++++++++++++++++++++++++++++

Comment on the overall structure of this file now I've been trying
to navigate through it for a little while. It is structured like:

<some attri stuff>
<some attrd stuff>
static const struct xfs_item_ops xfs_attrd_item_ops = {...}
<some more attri stuff>
static const struct xfs_item_ops xfs_attri_item_ops = {...}
<some attri log recovery stuff>
<some attrd log recovery stuff>

IOWs, the attri and attrd functions are interleaved non-obvious
ways and that makes it hard to navigate around when trying to find
related information. It would make more sense to me to structure
this as:

<attri stuff>
<attri log recovery stuff>
<some attrd stuff>
<attrd log recovery stuff>
static const struct xfs_item_ops xfs_attri_item_ops = {...}
const struct xlog_recover_item_ops xlog_attri_item_ops = {...}
static const struct xfs_item_ops xfs_attrd_item_ops = {...}
const struct xlog_recover_item_ops xlog_attrd_item_ops = {...}

because then all the related functionality is grouped together. It
also puts all the ops structures together in the one place, so we
don't have to jump around all over the file when just looking at
what ops the items run at different times...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com