Re: [PATCH v3 02/17] Set up infastructure for deferred attribute operations

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

On 11/28/2017 06:19 PM, Dave Chinner wrote:

> On Tue, Nov 28, 2017 at 11:45:47AM -0800, Darrick J. Wong wrote:
>> On Fri, Nov 17, 2017 at 11:21:30AM -0700, Allison Henderson wrote:
>>> +/*
>>> + * This is the structure used to lay out an attr log item in the
>>> + * log.
>>> + */
>>> +struct xfs_attr_log_format {
>>> +	uint64_t	alf_id;		/* attri identifier */
>>> +	xfs_ino_t       alf_ino;	/* the inode for this attr operation */
>>> +	uint32_t        alf_op_flags;	/* marks the op as a set or remove */
>>> +	uint32_t        alf_name_len;	/* attr name length */
>>> +	uint32_t        alf_value_len;	/* attr value length */
>>> +	uint32_t        alf_attr_flags;	/* attr flags */
>>> +	uint16_t	alf_type;	/* attri log item type */
>>> +	uint16_t	alf_size;	/* size of this item */
>> Type and size should go first so that the self-identification
>> information ends up at the same byte offsets as the other log formats.
>> This makes it much easier to dissect dirty log contents by hand if
>> things get messy.
> I'll point out this is not a "nice to have" feature but a
> requirement of the on-disk log format structures.
>
> That is, log recovery assumes that every log format item it finds in
> the log has it's type and size as the first two 16 bit fields in the
> log format item so it can validate that a) it's a known log format
> type, and b) knows how big the log format structure it is about to
> decode is supposed to be.
>
>  From fs/xfs/xfs_log_recovery.c:
>
> /*
>   * The next region to add is the start of a new region.  It could be
>   * a whole region or it could be the first part of a new region.  Because
>   * of this, the assumption here is that the type and size fields of all
>   * format structures fit into the first 32 bits of the structure.
>   *
>   * This works because all regions must be 32 bit aligned.  Therefore, we
>   * either have both fields or we have neither field.  In the case we have
>   * neither field, the data part of the region is zero length.  We only have
>   * a log_op_header and can throw away the header since a new one will appear
>   * later.  If we have at least 4 bytes, then we can determine how many regions
>   * will appear in the current log item.
>   */
> STATIC int
> xlog_recover_add_to_trans(
> .....
>
> Also, see the use of the ITEM_TYPE() macro in
> fs/xfs/xfs_log_recovery.c as another example of assuming the type
> field is the first 16 bits of the log format structures....
>
>
> Cheers,
>
> Dave.
Alrighty, thank you for pointing that out, I will be sure to get it 
corrected in the next version.  And thanks everyone for the very 
thorough review!!

Allison