xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 07/32] xfs: dirent dtype presence is dependent on directory magic numbers
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Wed, 09 Oct 2013 15:57:51 -0500
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20131009205131.GK4446@dastard>
References: <1380510944-8571-1-git-send-email-david@xxxxxxxxxxxxx> <1380510944-8571-8-git-send-email-david@xxxxxxxxxxxxx> <525495A3.30607@xxxxxxxxxxx> <20131009205131.GK4446@dastard>
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:17.0) Gecko/20130801 Thunderbird/17.0.8
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@xxxxxxxxxx>
>>>
>>> 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.
> 

<Prev in Thread] Current Thread [Next in Thread>