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@xxxxxxxxxx>
> > > >
> ...
> >
> > > 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@xxxxxxxxxxxxx
|