xfs
[Top] [All Lists]

Re: [PATCH 2/10] xfs: xfs_bulkstat_single consolidation

To: Brian Foster <bfoster@xxxxxxxxxx>, "xfs@xxxxxxxxxxx" <xfs@xxxxxxxxxxx>
Subject: Re: [PATCH 2/10] xfs: xfs_bulkstat_single consolidation
From: Jeff Liu <jeff.liu@xxxxxxxxxx>
Date: Sat, 04 Jan 2014 21:46:00 +0800
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <52C7232B.3050807@xxxxxxxxxx>
References: <52BEB3E7.2080706@xxxxxxxxxx> <52C7232B.3050807@xxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.1.0
On 01/04 2014 04:52 AM, Brian Foster wrote:
> On 12/28/2013 06:20 AM, Jeff Liu wrote:
>> From: Jie Liu <jeff.liu@xxxxxxxxxx>
>>
>> In xfs_bulkstat_single(), xfs_bulkstat_one() and xfs_bulkstat() might
>> return different error if either call failed, we'd better return the
>> proper error in this case.  Moreover, the function argument done is
>> useless in terms of xfs_ioc_bulkstat(), hence we can get rid of it.
>>
>> Signed-off-by: Jie Liu <jeff.liu@xxxxxxxxxx>
>> ---
>>  fs/xfs/xfs_ioctl.c  |  3 +--
>>  fs/xfs/xfs_itable.c | 59 
>> ++++++++++++++++++++++++++---------------------------
>>  fs/xfs/xfs_itable.h |  3 +--
>>  3 files changed, 31 insertions(+), 34 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
>> index 33ad9a7..2fdd750 100644
>> --- a/fs/xfs/xfs_ioctl.c
>> +++ b/fs/xfs/xfs_ioctl.c
>> @@ -828,8 +828,7 @@ xfs_ioc_bulkstat(
>>              error = xfs_inumbers(mp, &inlast, &count,
>>                                      bulkreq.ubuffer, xfs_inumbers_fmt);
>>      else if (cmd == XFS_IOC_FSBULKSTAT_SINGLE)
>> -            error = xfs_bulkstat_single(mp, &inlast,
>> -                                            bulkreq.ubuffer, &done);
>> +            error = xfs_bulkstat_single(mp, &inlast, bulkreq.ubuffer);
>>      else    /* XFS_IOC_FSBULKSTAT */
>>              error = xfs_bulkstat(mp, &inlast, &count, xfs_bulkstat_one,
>>                                   sizeof(xfs_bstat_t), bulkreq.ubuffer,
>> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
>> index b5bb7b6..692671c 100644
>> --- a/fs/xfs/xfs_itable.c
>> +++ b/fs/xfs/xfs_itable.c
>> @@ -509,51 +509,50 @@ xfs_bulkstat(
>>  }
>>  
>>  /*
>> - * Return stat information in bulk (by-inode) for the filesystem.
>> - * Special case for non-sequential one inode bulkstat.
>> + * Return stat information in bulk (by-inode) for the filesystem.  Special 
>> case
>> + * for non-sequential one inode bulkstat.
>>   */
>> -int                                 /* error status */
>> +int
>>  xfs_bulkstat_single(
>> -    xfs_mount_t             *mp,    /* mount point for filesystem */
>> +    struct xfs_mount        *mp,    /* mount point for filesystem */
>>      xfs_ino_t               *lastinop, /* inode to return */
>> -    char                    __user *buffer, /* buffer with inode stats */
>> -    int                     *done)  /* 1 if there are more stats to get */
>> +    char                    __user *buffer) /* buffer with inode stats */
>>  {
>> -    int                     count;  /* count value for bulkstat call */
>> -    int                     error;  /* return value */
>> -    xfs_ino_t               ino;    /* filesystem inode number */
>> +    xfs_ino_t               ino = *lastinop;
>>      int                     res;    /* result from bs1 */
>> +    int                     error;
>>  
>>      /*
>> -     * note that requesting valid inode numbers which are not allocated
>> +     * Note that requesting valid inode numbers which are not allocated
>>       * to inodes will most likely cause xfs_imap_to_bp to generate warning
>> -     * messages about bad magic numbers. This is ok. The fact that
>> -     * the inode isn't actually an inode is handled by the
>> -     * error check below. Done this way to make the usual case faster
>> -     * at the expense of the error case.
>> +     * messages about bad magic numbers. This is ok. The fact that the
>> +     * inode isn't actually an inode is handled by the error check below.
>> +     * Done this way to make the usual case faster at the expense of the
>> +     * error case.
>>       */
>> -
>> -    ino = *lastinop;
>> -    error = xfs_bulkstat_one(mp, ino, buffer, sizeof(xfs_bstat_t),
>> +    error = xfs_bulkstat_one(mp, ino, buffer, sizeof(struct xfs_bstat),
>>                               NULL, &res);
>>      if (error) {
>> +            int             count = 1;
>> +            int             done, error2;
>> +
>>              /*
>> -             * Special case way failed, do it the "long" way
>> -             * to see if that works.
>> +             * Special case way failed, do it the "long" way to see if
>> +             * that works.
>>               */
>>              (*lastinop)--;
>> -            count = 1;
>> -            if (xfs_bulkstat(mp, lastinop, &count, xfs_bulkstat_one,
>> -                            sizeof(xfs_bstat_t), buffer, done))
>> -                    return error;
>> -            if (count == 0 || (xfs_ino_t)*lastinop != ino)
>> -                    return error == EFSCORRUPTED ?
>> -                            XFS_ERROR(EINVAL) : error;
>> -            else
>> -                    return 0;
>> +            error2 = xfs_bulkstat(mp, lastinop, &count, xfs_bulkstat_one,
>> +                                  sizeof(struct xfs_bstat), buffer, &done);
>> +            if (error2)
>> +                    return error2;
>> +
>> +            if (count == 0 || *lastinop != ino) {
>> +                    if (error == EFSCORRUPTED)
>> +                            error = XFS_ERROR(EINVAL);
>> +            }
>>      }
>> -    *done = 0;
>> -    return 0;
>> +
>> +    return error;
> 
> Here it looks like you return an error value if the initial
> xfs_bulkstat_one() fails and the fallback xfs_bulkstat() succeeds.
Good catch, thanks for for your review.

Thanks,
-Jeff

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