xfs
[Top] [All Lists]

Re: [PATCH 1/2] xfs: Remove XFS_MOUNT_RETERR

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/2] xfs: Remove XFS_MOUNT_RETERR
From: Jeff Liu <jeff.liu@xxxxxxxxxx>
Date: Thu, 02 May 2013 14:55:08 +0800
Cc: "xfs@xxxxxxxxxxx" <xfs@xxxxxxxxxxx>, Mark Tinguely <tinguely@xxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20130502011211.GR10481@dastard>
References: <518125C9.8000008@xxxxxxxxxx> <20130502011211.GR10481@dastard>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:16.0) Gecko/20121028 Thunderbird/16.0.2
On 05/02/2013 09:12 AM, Dave Chinner wrote:
> On Wed, May 01, 2013 at 10:25:13PM +0800, Jeff Liu wrote:
>> From: Jie Liu <jeff.liu@xxxxxxxxxx>
>>
>> XFS_MOUNT_RETERR is going to be set at xfs_parseargs() if
>> mp->m_dalign is enabled, so any time we enter "if (mp->m_dalign)"
>> branch in xfs_update_alignment(), XFS_MOUNT_RETERR is set and
>> so we always be emitting a warning and returning an error.
>>
>> Hence, we can remove it and get rid of a couple of redundant
>> check up against it at xfs_upate_alignment(). 
>>
>> Thanks Dave Chinner for the confirmation.
>>
>> Signed-off-by: Jie Liu <jeff.liu@xxxxxxxxxx>
>> Cc: Dave Chinner <dchinner@xxxxxxxxxx>
>> Cc: Mark Tinguely <tinguely@xxxxxxx>
>> ---
>>  fs/xfs/xfs_mount.c |   39 ++++++++++++---------------------------
>>  fs/xfs/xfs_mount.h |    2 --
>>  fs/xfs/xfs_super.c |    5 +----
>>  3 files changed, 13 insertions(+), 33 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
>> index 3806088..29e8de8 100644
>> --- a/fs/xfs/xfs_mount.c
>> +++ b/fs/xfs/xfs_mount.c
>> @@ -872,42 +872,27 @@ xfs_update_alignment(xfs_mount_t *mp)
>>               */
>>              if ((BBTOB(mp->m_dalign) & mp->m_blockmask) ||
>>                  (BBTOB(mp->m_swidth) & mp->m_blockmask)) {
>> -                    if (mp->m_flags & XFS_MOUNT_RETERR) {
>> -                            xfs_warn(mp, "alignment check failed: "
>> -                                     "(sunit/swidth vs. blocksize)");
>> -                            return XFS_ERROR(EINVAL);
>> -                    }
>> -                    mp->m_dalign = mp->m_swidth = 0;
>> +                    xfs_warn(mp, "alignment check failed: "
>> +                             "(sunit/swidth vs. blocksize)");
> 
> Let's convert these format strings to a single line at the same
> time and be consistent with output. ie:
> 
>                       xfs_warn(mp,
>       "alignment check failed: sunit/swidth vs. blocksize(%d)",
>                               sbp->sb_blocksize);
Ok.
> 
>> +                    return XFS_ERROR(EINVAL);
>>              } else {
>>                      /*
>>                       * Convert the stripe unit and width to FSBs.
>>                       */
>>                      mp->m_dalign = XFS_BB_TO_FSBT(mp, mp->m_dalign);
>>                      if (mp->m_dalign && (sbp->sb_agblocks % mp->m_dalign)) {
>> -                            if (mp->m_flags & XFS_MOUNT_RETERR) {
>> -                                    xfs_warn(mp, "alignment check failed: "
>> -                                             "(sunit/swidth vs. ag size)");
>> -                                    return XFS_ERROR(EINVAL);
>> -                            }
>> -                            xfs_warn(mp,
>> -            "stripe alignment turned off: sunit(%d)/swidth(%d) "
>> -            "incompatible with agsize(%d)",
>> -                                    mp->m_dalign, mp->m_swidth,
>> -                                    sbp->sb_agblocks);
>> -
>> -                            mp->m_dalign = 0;
>> -                            mp->m_swidth = 0;
>> +                            xfs_warn(mp, "alignment check failed: "
>> +                    "sunit(%d)/swidth(%d) incompatible with agsize(%d)",
> 
>       "alignment check failed: sunit/swidth vs. agsize(%d)",
>                                       sbp->sb_agblocks);
> 
>> +                                     mp->m_dalign, mp->m_swidth,
>> +                                     sbp->sb_agblocks);
>> +                            return XFS_ERROR(EINVAL);
>>                      } else if (mp->m_dalign) {
>>                              mp->m_swidth = XFS_BB_TO_FSBT(mp, mp->m_swidth);
>>                      } else {
>> -                            if (mp->m_flags & XFS_MOUNT_RETERR) {
>> -                                    xfs_warn(mp, "alignment check failed: "
>> -                                            "sunit(%d) less than bsize(%d)",
>> -                                            mp->m_dalign,
>> -                                            mp->m_blockmask +1);
>> -                                    return XFS_ERROR(EINVAL);
>> -                            }
>> -                            mp->m_swidth = 0;
>> +                            xfs_warn(mp, "alignment check failed: "
>> +                                     "sunit(%d) less than bsize(%d)",
>> +                                     mp->m_dalign, mp->m_blockmask + 1);
> 
>       "alignment check failed: sunit(%d) less than bsize(%d)",
>                                       mp->m_dalign, sbp->sb_blocksize);
>> +                            return XFS_ERROR(EINVAL);
>>                      }
>>              }
>>  
>> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
>> index bc90706..8145412 100644
>> --- a/fs/xfs/xfs_mount.h
>> +++ b/fs/xfs/xfs_mount.h
>> @@ -230,8 +230,6 @@ typedef struct xfs_mount {
>>                                                 operations, typically for
>>                                                 disk errors in metadata */
>>  #define XFS_MOUNT_DISCARD   (1ULL << 5)     /* discard unused blocks */
>> -#define XFS_MOUNT_RETERR    (1ULL << 6)     /* return alignment errors to
>> -                                               user */
>>  #define XFS_MOUNT_NOALIGN   (1ULL << 7)     /* turn off stripe alignment
>>                                                 allocations */
>>  #define XFS_MOUNT_ATTR2             (1ULL << 8)     /* allow use of attr2 
>> format */
>> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
>> index ea341ce..51da4d2 100644
>> --- a/fs/xfs/xfs_super.c
>> +++ b/fs/xfs/xfs_super.c
>> @@ -446,11 +446,8 @@ done:
>>               * Before the mount call ends we will convert
>>               * these to FSBs.
>>               */
>> -            if (dsunit) {
>> +            if (dsunit)
>>                      mp->m_dalign = dsunit;
>> -                    mp->m_flags |= XFS_MOUNT_RETERR;
>> -            }
>> -
>>              if (dswidth)
>>                      mp->m_swidth = dswidth;
> 
> This code can be simplified, too. We've already done this check:
> 
>       if ((dsunit && !dswidth) || (!dsunit && dswidth)) {
>                 xfs_warn(mp, "sunit and swidth must be specified together");
>                 return EINVAL;
>         }
> 
> So we know that dsunit/dswidth are either both zero or both set, so
> the above code could simply end up as:
> 
>         if (dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) {
>                 /*
>                  * At this point the superblock has not been read
>                  * in, therefore we do not know the block size.
>                  * Before the mount call ends we will convert
>                  * these to FSBs.
>                  */
>               mp->m_dalign = dsunit;
>               mp->m_swidth = dswidth;
>         }
Exactly, will take care of this in next round of post.

Thanks,
-Jeff

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