On Wed, Nov 02, 2022 at 01:41:25AM -0700, Christoph Hellwig wrote:
> > + *((int *)&iomap->private) = sequence;
>
> > +static bool
> > +xfs_buffered_write_iomap_valid(
> > + struct inode *inode,
> > + const struct iomap *iomap)
> > +{
> > + int seq = *((int *)&iomap->private);
>
> I really hate this stuffing of the sequence into the private pointer.
Oh, I'm no fan of it either. It was this or work out how to support
sequence numbers/cookies directly in iomaps...
> The iomap structure isn't so size constrained that we have to do that,
> so we can just add a sequence number field directly to it. I don't
> think that is a layering violation, as the concept of a sequence
> numebr is pretty generic and we'll probably need it for all file systems
> eventually.
*nod*
This was the least of my worries trying to get this code to work. I
didn't have to think about it this way, so it was one less thing to
worry about.
My concerns with putting it into the iomap is that different
filesystems will have different mechanisms for detecting stale
iomaps. THe way we do it with a generation counter is pretty coarse
as any change to the extent map will invalidate the iomap regardless
of whether they overlap or not.
If, in future, we want something more complex and finer grained
(e.g. an iext tree cursor) to allow us to determine if the
change to the extent tree actually modified the extent backing the
iomap, then we are going to need an opaque cookie of some kind, not
a u32 or a u32*.
> > +
> > + if (seq != READ_ONCE(XFS_I(inode)->i_df.if_seq))
> > + return false;
>
> Which makes me wonder if we could do away with the callback entirely
> by adding an option sequence number pointer to the iomap_iter. If set
> the core code compares it against iomap->seq and we get rid of the
> per-folio indirect call, and boilerplate code that would need to be
> implemented in every file system.
I'm not convinced that this is the right way to proceed.
The writeback code also has cached iomap validity checks via a
callback. The checks in xfs_imap_valid() require more than just a
single sequence number check - there are two sequence numbers, one
of which is conditionally checked when the inode may have COW
operations occurring.
Hence I don't think that encoding a single u32 into the iomap is
generic enough even for the current "check the cached iomap is not
stale" use cases we have. I'd really like to move towards a common
mechanism for both the write and writeback paths.
I didn't have the brain capacity to think all this through at the
time, so I just stuffed the sequence number in the private field and
moved on to the next part of the problem.
Indeed, I haven't even checked if the read path might be susceptible
to stale cached iomaps yet....
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
