Re: [PATCH 07/32] xfs: dirent dtype presence is dependent on directory magic numbers

[Eric Sandeen <sandeen@sandeen.net>]

On 10/9/13 3:51 PM, Dave Chinner wrote:
> On Tue, Oct 08, 2013 at 06:30:43PM -0500, Eric Sandeen wrote:
>> On 9/29/13 10:15 PM, Dave Chinner wrote:
>>> From: Dave Chinner <dchinner@redhat.com>
>>>
>>> The determination of whether a directory entry contains a dtype
>>> field originally was dependent on the filesystem having CRCs
>>> enabled. This meant that the format for dtype beign enabled could be
>>> determined by checking the directory block magic number rather than
>>> doing a feature bit check. This was useful in that it meant that we
>>> didn't need to pass a struct xfs_mount around to functions that
>>> were already supplied with a directory block header.
>>>
>>> Unfortunately, the introduction of dtype fields into the v4
>>> structure via a feature bit meant this "use the directory block
>>> magic number" method of discriminating the dirent entry sizes is
>>> broken. Hence we need to convert the places that use magic number
>>> checks to use feature bit checks so that they work correctly and not
>>> by chance.
>>>
>>> The current code works on v4 filesystems only because the dirent
>>> size roundup covers the extra byte needed by the dtype field in the
>>> places where this problem occurs.
>>
>> Looks right to me.  Nitpicks & questions though:
>>
>> FWIW, I find it confusing that we call xfs_dir3_*()
>> functions from dir2 code or to find out whether the
>> dir is in fact dir2 or dir3.
>>
>> i.e.:
>>
>> return xfs_dir3_data_hdr_size(xfs_sb_version_hascrc(&mp->m_sb));
> 
> It's the convention I've used since first introducing the CRC code.
> dir2 means it handles just dir2 format, dir3 means it handles either
> the dir2 or the CRC enabled format.
> 
> That said, this goes away in the directory ops vectorisation patch
> set, when dir2 means "handles dir2 format" and dir3 means "handles
> only dir3 format"....
> 
>> that just seems like an odd name to calculate the header size for
>> dir2 vs. dir3 directories.
>>
>> Also -
>>
>> Is there any pro or con to defining the 3 offsets recursively:
>>
>>  static inline xfs_dir2_data_aoff_t
>>  xfs_dir3_data_dot_offset(struct xfs_mount *mp)
>>  {
>>  	return xfs_dir3_data_hdr_size(xfs_sb_version_hascrc(&mp->m_sb));
>>  }
>>  
>>  static inline xfs_dir2_data_aoff_t
>>  xfs_dir3_data_dotdot_offset(struct xfs_mount *mp)
>>  {
>>  	return xfs_dir3_data_dot_offset(mp) +
>> 		xfs_dir3_data_entsize(mp, 1);
>>  }
>>  
>>  static inline xfs_dir2_data_aoff_t
>>  xfs_dir3_data_first_offset(struct xfs_mount *mp)
>>  {
>> 	return xfs_dir3_data_dotdot_offset(mp) +
>> 		xfs_dir3_data_entsize(mp, 2);
>>  }
>>
>> vs directly, i.e.:
>>
>>  static inline xfs_dir2_data_aoff_t
>>  xfs_dir3_data_first_offset(struct xfs_mount *mp)
>>  {
>> 	return xfs_dir3_data_hdr_size(xfs_sb_version_hascrc(&mp->m_sb)) +
>> 		xfs_dir3_data_entsize(mp, 1);	/* Dot */
>> 		xfs_dir3_data_entsize(mp, 2);	/* Dotdot */
>>  }
> 
> None, really. This is just a mechanical change to fix the bug, not a
> change of logic. This is changed to the direct method in the dir ops
> vectorisation series....

Ok.  Sorry for not keeping up.

(I knew it was mechanical, didn't mean this patch was wrong, just wondering
out loud some more).

-Eric

> Cheers,
> 
> Dave.
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs