xfs
[Top] [All Lists]

Re: [PATCH 3/3] xfs: byte range granularity for XFS_IOC_ZERO_RANGE

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 3/3] xfs: byte range granularity for XFS_IOC_ZERO_RANGE
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Fri, 23 Nov 2012 19:59:30 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20121123083403.GB24938@xxxxxxxxxxxxx>
References: <1353641065-14983-1-git-send-email-david@xxxxxxxxxxxxx> <1353641065-14983-4-git-send-email-david@xxxxxxxxxxxxx> <20121123083403.GB24938@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Fri, Nov 23, 2012 at 03:34:03AM -0500, Christoph Hellwig wrote:
> On Fri, Nov 23, 2012 at 02:24:25PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> > 
> > XFS_IOC_ZERO_RANGE simply does not work properly for non page cache
> > aligned ranges. Neither test 242 or 290 exercise this correctly, so
> > the behaviour is completely busted even though the tests pass.
> > 
> > Fix it to support full byte range granularity as was originally
> > intended for this ioctl.
> 
> Looks good, but a couple cosmetic comments below:
> 
> > +   rounding = max_t(uint, 1 << mp->m_sb.sb_blocklog, PAGE_CACHE_SIZE);
> 
> I'd call this granularity.

OK. I just copied that from the hole punch code. ;)

> > +   /* round the range iof extents we are going to convert inwards */
> > +   start = round_up(offset, rounding);
> > +   end = round_down(offset + len, rounding);
> 
> start_boundary, end_boundary?

OK.

> > +   if (start < end - 1) {
> > +           /* punch out the page cache over the conversion range */
> > +           truncate_pagecache_range(VFS_I(ip), start, end - 1);
> > +           /* convert the blocks */
> > +           error = xfs_alloc_file_space(ip, start, end - start - 1,
> > +                               XFS_BMAPI_PREALLOC | XFS_BMAPI_CONVERT,
> > +                               attr_flags);
> > +           if (error)
> > +                   goto out_unlock;
> > +   } else {
> > +           /* it's a sub-rounding range */
> > +           ASSERT(offset + len <= start);
> > +           error = xfs_iozero(ip, offset, len);
> > +           goto out_unlock;
> > +   }
> > +
> > +   /* now we've handled the interior of the range, handle the edges */
> > +   if (start != offset)
> > +           error = xfs_iozero(ip, offset, start - offset);
> > +   if (!error && end != offset + len)
> > +           error = xfs_iozero(ip, end, offset + len - end);
> 
> I'd move the edge iozero calls into the if branch, that gives a natural
> code flow and avoids the goto unlock in the sub-page case.

Sure. I added the sub-page case after the edge cases when I realised
the edge cases didn't handle that. I'll rework it.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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