Re: [PATCH 2/6] xfs: introduce BMAPI_ZERO for allocating zeroed extents

[Dave Chinner <david@fromorbit.com>]

On Fri, Oct 30, 2015 at 08:36:08AM -0400, Brian Foster wrote:
> On Fri, Oct 30, 2015 at 10:35:48AM +1100, Dave Chinner wrote:
> > On Thu, Oct 29, 2015 at 10:27:58AM -0400, Brian Foster wrote:
> > > On Mon, Oct 19, 2015 at 02:27:14PM +1100, Dave Chinner wrote:
> > > > From: Dave Chinner <dchinner@redhat.com>
> > > > 
> ...
> > 
> > > I suspect there's no use case to pass XFS_BMAPI_ZERO for new unwritten
> > > allocations, but if the flag is passed, doesn't this cause duplicate
> > > block zeroing?
> > 
> > It probably would, but....
> > 
> > > Perhaps we should drop the zero flag from 'flags' after
> > > allocation in xfs_bmapi_write() just to ensure this executes in one
> > > place or the other..?
> > 
> > I think that if we hit this, we're doing something else wrong - why
> > would we allocate unwritten extents and still need to initialise
> > them to zero?
> > 
> 
> No idea, really (as noted above). ;) It just looked like it could be
> invoked twice per bmapi call, nothing else I saw prevented it, and it
> looks easily avoidable. Maybe somebody down the road decides to turn on
> block zeroing unconditionally in the block allocator due to hardware
> support or some such. Or maybe we'll never hit the problem. The point is
> that this code will inevitably be modified/enhanced down the road and
> nobody is going to remember that the zeroing is invoked twice in a
> particular prealloc codepath.
> 
> If we don't want to mess with the flags, how about an assert somewhere
> so it's explicit the bmapi implementation doesn't expect this
> combination of flags?

Easy enough. I'll add this to the initial asserts in xfs_bmapi_write():

+	/*
+	 * we can allocate unwritten extents or pre-zero allocated blocks,
+	 * but it makes no sense to do both at once. This would result in
+	 * zeroing the unwritten extent twice, but it still being an
+	 * unwritten extent....
+	 */
+	ASSERT((flags & (XFS_BMAPI_PREALLOC|XFS_BMAPI_ZERO)) !=
+			(XFS_BMAPI_PREALLOC|XFS_BMAPI_ZERO));

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs