xfs
[Top] [All Lists]

Re: [PATCH v2 05/10] xfs: fix error handling in xfs_bulkstat

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH v2 05/10] xfs: fix error handling in xfs_bulkstat
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Mon, 28 Apr 2014 07:26:44 +1000
Cc: Jeff Liu <jeff.liu@xxxxxxxxxx>, "xfs@xxxxxxxxxxx" <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20140425064815.GB20871@xxxxxxxxxxxxx>
References: <535078B8.4020905@xxxxxxxxxx> <20140425064815.GB20871@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Thu, Apr 24, 2014 at 11:48:15PM -0700, Christoph Hellwig wrote:
> > Moreover, this fix also get rid of the redundant user buffer count
> > pre-checkups as it has already been validated in upper callers.
> 
> > -   if (!ubcountp || *ubcountp <= 0) {
> > -           return EINVAL;
> > -   }
> 
> Probably better to have this as a separate patch.
> 
> > -                   /*
> > -                    * Loop as long as we're unable to read the
> > -                    * inode btree.
> > -                    */
> > -                   while (error) {
> > -                           agino += XFS_INODES_PER_CHUNK;
> > -                           if (XFS_AGINO_TO_AGBNO(mp, agino) >=
> > -                                           be32_to_cpu(agi->agi_length))
> > -                                   break;
> > -                           error = xfs_inobt_lookup(cur, agino,
> > -                                                    XFS_LOOKUP_GE, &tmp);
> > -                           cond_resched();
> > -                   }
> 
> This code goes back to 1995, but I still can't see how it would make
> sense.  I think we should get rid of this, but I'd also love to have
> Dave and Eric double check it as well.

I can't see it makes much sense, except for handling IO errors
that occur during something like a path failover where a retry would
then succeed. However, I think that we'd do better for userspace to
handle this problem - a short bulkstat followed by userspace retry
rather than a potential endless loop in the kernel is a much better
way to handle the problem...

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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