On Fri, Dec 13, 2024 at 02:37:57PM -0800, Darrick J. Wong wrote:
> > index d35ac4c19fb2..67392413216b 100644
> > --- a/fs/xfs/xfs_aops.c
> > +++ b/fs/xfs/xfs_aops.c
> > @@ -1,7 +1,7 @@
> > // SPDX-License-Identifier: GPL-2.0
> > /*
> > * Copyright (c) 2000-2005 Silicon Graphics, Inc.
> > - * Copyright (c) 2016-2018 Christoph Hellwig.
> > + * Copyright (c) 2016-2023 Christoph Hellwig.
>
> 2024, surely?
>
> Or at this point, 2025
Nost of this actually was done (and pushed to a semi-public tree) in
2023. But I guess I touched it enough in 2024 to cover that.
> > + wpc->iomap.type = IOMAP_MAPPED;
> > + wpc->iomap.flags = IOMAP_F_DIRTY;
> > + wpc->iomap.bdev = mp->m_rtdev_targp->bt_bdev;
> > + wpc->iomap.offset = offset;
> > + wpc->iomap.length = XFS_FSB_TO_B(mp, count_fsb);
> > + wpc->iomap.flags = IOMAP_F_ZONE_APPEND;
> > + wpc->iomap.addr = 0;
>
> /me wonders if this should be set somewhere other than block 0 just in
> case we screw up? That might just be paranoia since I think iomap puts
> the bio if it doesn't get to ->submit_bio.
I'll give it a spin.
> > + struct xfs_inode *ip = XFS_I(mapping->host);
> > struct xfs_writepage_ctx wpc = { };
> > + int error;
> >
> > - xfs_iflags_clear(XFS_I(mapping->host), XFS_ITRUNCATED);
> > + xfs_iflags_clear(ip, XFS_ITRUNCATED);
> > + if (xfs_is_zoned_inode(ip)) {
> > + struct xfs_zoned_writepage_ctx xc = { };
>
> I noticed that the zoned writepage ctx doesn't track data/cow fork
> sequence numbers. Why is this not necessary? Can we not race with
> someone else doing writeback?
Unlike "regular" XFS writeback, which tries to convert large extents from
preallocations (or just large ranges), zoned writeback works a single
folio at time, that there is a new mapping for each folio. And these are
only dummy mappings anyway. So all work happens on folios that are
locked or have the writeback bit set and are off limits to other
writeback threads, new buffered writers or invalidation. I guess I
need to document this better.
> > +xfs_zoned_buffered_write_iomap_begin(
> > + struct inode *inode,
> > + loff_t offset,
> > + loff_t count,
> > + unsigned flags,
> > + struct iomap *iomap,
> > + struct iomap *srcmap)
> > +{
> > + struct iomap_iter *iter =
> > + container_of(iomap, struct iomap_iter, iomap);
>
> I still wonder if iomap should be passing a (const struct iomap_iter *)
> to the ->iomap_begin function instead of passing all these variables and
> then implementations have to container_of if they want iter->private
> too.
It should, and I plan to pass the iter when reworking this towards
the iter model. Hopefully I can get to that relatively soon after
this project lands.
