xfs
[Top] [All Lists]

Re: [PATCH v2 03/10] xfs: consolidate xfs_inumbers

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH v2 03/10] xfs: consolidate xfs_inumbers
From: Jeff Liu <jeff.liu@xxxxxxxxxx>
Date: Sun, 04 May 2014 20:32:45 +0800
Cc: "xfs@xxxxxxxxxxx" <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20140423153639.GB3326@xxxxxxxxxxxxx>
References: <535078B2.8010601@xxxxxxxxxx> <20140423153639.GB3326@xxxxxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.1.0
Hi Christoph,

Sorry for my too late response! I missed your response somehow.

On 04/23 2014 23:36 PM, Christoph Hellwig wrote:
> On Fri, Apr 18, 2014 at 08:58:26AM +0800, Jeff Liu wrote:
>> From: Jie Liu <jeff.liu@xxxxxxxxxx>
>>
>> To fetch the file system number tables, we currently just ignore the
>> errors and proceed to loop over the next AG or bump agino to the next
>> chunk in case of btree operations failed, that is not properly because
>> those errors might hint us potential file system problems.
>>
>> This patch rework xfs_inumbers() to handle the btree operation errors
>> as well as the loop conditions.  Also, add pre-checkups for the given
>> inode, we can save alloc/free the format buffer once against an invalid
>> inode number.
> 
> The patch looks mostly good to me, but I really think it should be
> split into two patches: one to do the formatting changes and code
> consolidation, and then one that does the actual logic changes for
> better error handling.  It's not easy to understand and verify with
> these two different changes combined.
> 
>>  xfs_inumbers_fmt(
>>      void                    __user *ubuffer, /* buffer to write to */
>> -    const xfs_inogrp_t      *buffer,        /* buffer to read from */
>> +    const struct xfs_inogrp *buffer,        /* buffer to read from */
>>      long                    count,          /* # of elements to read */
>>      long                    *written)       /* # of bytes written */
>>  {
>>      if (copy_to_user(ubuffer, buffer, count * sizeof(*buffer)))
>> -            return -EFAULT;
>> +            return XFS_ERROR(EFAULT);
> 
> xfs_inumbers_fmt_compat will need the same treatment.

Yup.

> 
>>      *count = 0;
>> +    if (agno >= mp->m_sb.sb_agcount ||
>> +        *lastino != XFS_AGINO_TO_INO(mp, agno, agino))
>> +            return 0;
> 
> Where is the lastino check coming from?

Originally, I copied this check up from xfs_bulkstat(), it seems that it
is a redundant check as agno >= mp->m_sb.sb_agcount can handle an invalid
lastino input?  At least, I can not think out a case to make it happen for
now.

> 
>>      buffer = kmem_alloc(bcount * sizeof(*buffer), KM_SLEEP);
>> +    bufidx = error = 0;
> 
> Why not initialize bufidx and error at declaration time?

So they should be initialized there.

> 
>> +            error = xfs_inobt_get_rec(cur, &r, &stat);
>> +            if (error || !stat)
>> +                    break;
> 
> The old code moved on to the next AG here, why has this changed?

Ah, it should be the old way.


Thanks,
-Jeff

<Prev in Thread] Current Thread [Next in Thread>
  • Re: [PATCH v2 03/10] xfs: consolidate xfs_inumbers, Jeff Liu <=