Re: [PATCH 3/3] design: document extended attribute log item changes

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Jan 24, 2023 at 11:00:56AM +0530, Chandan Babu R wrote:
> On Tue, Jan 17, 2023 at 04:45:20 PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > Describe the changes to the ondisk log format that are required to
> > support atomic updates to extended attributes.
> >
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  .../allocation_groups.asciidoc                     |   14 ++-
> >  .../journaling_log.asciidoc                        |  109 ++++++++++++++++++++
> >  design/XFS_Filesystem_Structure/magic.asciidoc     |    2 
> >  3 files changed, 122 insertions(+), 3 deletions(-)
> >
> >
> > diff --git a/design/XFS_Filesystem_Structure/allocation_groups.asciidoc b/design/XFS_Filesystem_Structure/allocation_groups.asciidoc
> > index c64b4fad..c0ba16a8 100644
> > --- a/design/XFS_Filesystem_Structure/allocation_groups.asciidoc
> > +++ b/design/XFS_Filesystem_Structure/allocation_groups.asciidoc
> > @@ -461,9 +461,17 @@ space mappings allowed in data and extended attribute file forks.
> >  |=====
> >  
> >  *sb_features_log_incompat*::
> > -Read-write incompatible feature flags for the log.  The kernel cannot read or
> > -write this FS log if it doesn't understand the flag.  Currently, no flags are
> > -defined.
> > +Read-write incompatible feature flags for the log.  The kernel cannot recover
> > +the FS log if it doesn't understand the flag.
> > +
> > +.Extended Version 5 Superblock Log incompatibility flags
> > +[options="header"]
> > +|=====
> > +| Flag					| Description
> > +| +XFS_SB_FEAT_INCOMPAT_LOG_XATTRS+	|
> > +Extended attribute updates have been committed to the ondisk log.
> > +
> > +|=====
> >  
> >  *sb_crc*::
> >  Superblock checksum.
> > diff --git a/design/XFS_Filesystem_Structure/journaling_log.asciidoc b/design/XFS_Filesystem_Structure/journaling_log.asciidoc
> > index ddcb87f4..f36dd352 100644
> > --- a/design/XFS_Filesystem_Structure/journaling_log.asciidoc
> > +++ b/design/XFS_Filesystem_Structure/journaling_log.asciidoc
> > @@ -215,6 +215,8 @@ magic number to distinguish themselves.  Buffer data items only appear after
> >  | +XFS_LI_CUD+			| 0x1243        | xref:CUD_Log_Item[Reference Count Update Done]
> >  | +XFS_LI_BUI+			| 0x1244        | xref:BUI_Log_Item[File Block Mapping Update Intent]
> >  | +XFS_LI_BUD+			| 0x1245        | xref:BUD_Log_Item[File Block Mapping Update Done]
> > +| +XFS_LI_ATTRI+		| 0x1246        | xref:ATTRI_Log_Item[Extended Attribute Update Intent]
> > +| +XFS_LI_ATTRD+		| 0x1247        | xref:ATTRD_Log_Item[Extended Attribute Update Done]
> >  |=====
> >  
> >  Note that all log items (except for transaction headers) MUST start with
> > @@ -712,6 +714,113 @@ Size of this log item.  Should be 1.
> >  *bud_bui_id*::
> >  A 64-bit number that binds the corresponding BUI log item to this BUD log item.
> >  
> > +[[ATTRI_Log_Item]]
> > +=== Extended Attribute Update Intent
> > +
> > +The next two operation types work together to handle atomic extended attribute
> > +updates.
> > +
> > +The lower byte of the +alfi_op_flags+ field is a type code indicating what sort
> > +of file block mapping operation we want.
> > +
> > +.Extended attribute update log intent types
> > +[options="header"]
> > +|=====
> > +| Value				| Description
> > +| +XFS_ATTRI_OP_FLAGS_SET+	| Set a key/value pair.
> > +| +XFS_ATTRI_OP_FLAGS_REMOVE+	| Remove a key/value pair.
> > +| +XFS_ATTRI_OP_FLAGS_REPLACE+	| Replace one key/value pair with another.
> > +|=====
> > +
> > +The ``extended attribute update intent'' operation comes first; it tells the
> > +log that XFS wants to update one of a file's extended attributes.  This record
> > +is crucial for correct log recovery because it enables us to spread a complex
> > +metadata update across multiple transactions while ensuring that a crash midway
> > +through the complex update will be replayed fully during log recovery.
> > +
> > +[source, c]
> > +----
> > +struct xfs_attri_log_format {
> > +     uint16_t                  alfi_type;
> > +     uint16_t                  alfi_size;
> > +     uint32_t                  __pad;
> > +     uint64_t                  alfi_id;
> > +     uint64_t                  alfi_ino;
> > +     uint32_t                  alfi_op_flags;
> > +     uint32_t                  alfi_name_len;
> > +     uint32_t                  alfi_value_len;
> > +     uint32_t                  alfi_attr_filter;
> > +};
> > +----
> > +
> > +*alfi_type*::
> > +The signature of an ATTRI operation, 0x1246.  This value is in host-endian
> > +order, not big-endian like the rest of XFS.
> > +
> > +*alfi_size*::
> > +Size of this log item.  Should be 1.
> > +
> > +*alfi_id*::
> > +A 64-bit number that binds the corresponding ATTRD log item to this ATTRI log
> > +item.
> > +
> > +*alfi_ino*::
> > +Inode number of the file being updated.
> > +
> > +*alfi_op_flags*::
> > +The operation being performed.  The lower byte must be one of the
> > ++XFS_ATTRI_OP_FLAGS_*+ flags defined above.  The upper bytes must be zero.
> > +
> > +*alfi_name_len*::
> > +Length of the name of the extended attribute.  This must not be zero.
> > +The attribute name itself is captured in the next log item.
> > +
> > +*alfi_value_len*::
> > +Length of the value of the extended attribute.  This must be zero for remove
> > +operations, and nonzero for set and replace operations.  The attribute value
> > +itself is captured in the log item immediately after the item containing the
> > +name.
> > +
> > +*alfi_attr_filter*::
> > +Attribute namespace filter flags.  This must be one of +ATTR_ROOT+,
> > ++ATTR_SECURE+, or +ATTR_INCOMPLETE+.
> > +
> > +[[ATTRD_Log_Item]]
> > +=== Completion of Extended Attribute Updates
> > +
> > +The ``extended attribute update done'' operation complements the ``extended
> > +attribute update intent'' operation.  This second operation indicates that the
> > +update actually happened, so that log recovery needn't replay the update.  The
> > +ATTRD and the actual updates are typically found in a new transaction following
> > +the transaction in which the ATTRI was logged.
> > +
> > +[source, c]
> > +----
> > +struct xfs_attrd_log_format {
> > +      __uint16_t               alfd_type;
> > +      __uint16_t               alfd_size;
> > +      __uint32_t               __pad;
> > +      __uint64_t               alfd_alf_id;
> > +};
> > +----
> > +
> > +*alfd_type*::
> > +The signature of an ATTRD operation, 0x1247.  This value is in host-endian
> > +order, not big-endian like the rest of XFS.
> > +
> > +*alfd_size*::
> > +Size of this log item.  Should be 1.
> > +
> > +*alfd_bui_id*::
> 
> The above should be "alfd_alf_id". Apart from that, the remaining
> changes appear to be correct.
> 
> Reviewed-by: Chandan Babu R <chandan.babu@oracle.com>

I'll fix that.  Thanks for the review!

--D

> -- 
> chandan
> 
> > +A 64-bit number that binds the corresponding ATTRI log item to this ATTRD log
> > +item.
> > +
> > +=== Extended Attribute Name and Value
> > +
> > +These regions contain the name and value components of the extended attribute
> > +being updated, as needed.  There are no magic numbers; each region contains the
> > +data and nothing else.
> > +
> >  [[Inode_Log_Item]]
> >  === Inode Updates
> >  
> > diff --git a/design/XFS_Filesystem_Structure/magic.asciidoc b/design/XFS_Filesystem_Structure/magic.asciidoc
> > index 9be26f82..a343271a 100644
> > --- a/design/XFS_Filesystem_Structure/magic.asciidoc
> > +++ b/design/XFS_Filesystem_Structure/magic.asciidoc
> > @@ -71,6 +71,8 @@ are not aligned to blocks.
> >  | +XFS_LI_CUD+			| 0x1243        |       | xref:CUD_Log_Item[Reference Count Update Done]
> >  | +XFS_LI_BUI+			| 0x1244        |       | xref:BUI_Log_Item[File Block Mapping Update Intent]
> >  | +XFS_LI_BUD+			| 0x1245        |       | xref:BUD_Log_Item[File Block Mapping Update Done]
> > +| +XFS_LI_ATTRI+		| 0x1246        |       | xref:ATTRI_Log_Item[Extended Attribute Update Intent]
> > +| +XFS_LI_ATTRD+		| 0x1247        |       | xref:ATTRD_Log_Item[Extended Attribute Update Done]
> >  |=====
> >  
> >  = Theoretical Limits