Re: [PATCH 2/3] xfs: go straight to real allocations for direct I/O COW writes

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Wed, Dec 07, 2016 at 02:46:34PM -0500, Brian Foster wrote:
> On Wed, Dec 07, 2016 at 08:37:09PM +0100, Christoph Hellwig wrote:
> > On Wed, Dec 07, 2016 at 02:00:09PM -0500, Brian Foster wrote:
> > > > -	if (ap->flags & XFS_BMAPI_COWFORK)
> > > > -		align = xfs_get_cowextsz_hint(ap->ip);
> > > > -	else if (xfs_alloc_is_userdata(ap->datatype))
> > > > +	if (xfs_alloc_is_userdata(ap->datatype))
> > > 
> > > Doesn't this defeat the purpose (i.e., fragmentation avoidance via wider
> > > allocations) of the cowextszhint for direct I/O? I think it would be
> > > better to be consistent with the approach for traditional I/O + extsz
> > > and incorporate the alignment into the reservation. Perhaps the hunk of
> > > code that already does just that in xfs_iomap_write_direct() could be
> > > converted to a small helper and reused..?
> > 
> > We're already doing the alignment to the cowextsize hint in
> > __xfs_reflink_allocate_cow so that we can take the cowextsize into
> > account.
> > 
> 
> Only for end_fsb... xfs_bmap_btalloc() calls xfs_bmap_extsize_align()
> with the alignment, which rounds out the start and end offsets.

Seconded; I finally got a chance to build a kernel with these three
patches and ran xfs/214 with a tracepointed kernel; it doesn't round the
start down to the previous cowextsize alignment like 4.9 does... and
thus the quota accounting is off.

--D

> 
> This same end_fsb code has already been removed from
> xfs_reflink_reserve_cow() for similar reasons. It should probably be
> removed from xfs_reflink_allocate_cow() as well.
> 
> Brian
> 
> > > > +	resblks = XFS_DIOSTRAT_SPACE_RES(mp, end_fsb - *offset_fsb);
> > > > +
> > > > +	error = xfs_qm_dqattach(ip, 0);
> > > 
> > > This is already in the (only) caller.
> > 
> > Yes, it can be dropped, although a superflous xfs_qm_dqattach is
> > totally harmless anyway.
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html