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

[Allison Henderson <allison.henderson@oracle.com>]

On 8/31/21 2:52 PM, Dave Chinner wrote:
> 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...
> 
Sure, will make a note to re-arrange some of these in the next version

Allison

> Cheers,
> 
> Dave.
>