| Message ID | 20240827051028.1751933-3-hch@lst.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [01/10] iomap: handle a post-direct I/O invalidate race in iomap_write_delalloc_release | expand |
On Tue, Aug 27, 2024 at 07:09:49AM +0200, Christoph Hellwig wrote: > Currently iomap_unshare_iter relies on the IOMAP_F_SHARED flag to detect > blocks to unshare. This is reasonable, but IOMAP_F_SHARED is also useful > for the file system to do internal book keeping for out of place writes. > XFS used to that, until it got removed in commit 72a048c1056a > ("xfs: only set IOMAP_F_SHARED when providing a srcmap to a write") > because unshare for incorrectly unshare such blocks. > > Add an extra safeguard by checking the explicitly provided srcmap instead > of the fallback to the iomap for valid data, as that catches the case > where we'd just copy from the same place we'd write to easily, allowing > to reinstate setting IOMAP_F_SHARED for all XFS writes that go to the > COW fork. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/iomap/buffered-io.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index 69a931de1979b9..737a005082e035 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -1337,16 +1337,25 @@ EXPORT_SYMBOL_GPL(iomap_file_buffered_write_punch_delalloc); > static loff_t iomap_unshare_iter(struct iomap_iter *iter) > { > struct iomap *iomap = &iter->iomap; > - const struct iomap *srcmap = iomap_iter_srcmap(iter); > loff_t pos = iter->pos; > loff_t length = iomap_length(iter); > loff_t written = 0; > > - /* don't bother with blocks that are not shared to start with */ > + /* Don't bother with blocks that are not shared to start with. */ > if (!(iomap->flags & IOMAP_F_SHARED)) > return length; > - /* don't bother with holes or unwritten extents */ > - if (srcmap->type == IOMAP_HOLE || srcmap->type == IOMAP_UNWRITTEN) > + > + /* > + * Don't bother with holes or unwritten extents. > + * > + * Note that we use srcmap directly instead of iomap_iter_srcmap as > + * unsharing requires providing a separate source map, and the presence > + * of one is a good indicator that unsharing is needed, unlike > + * IOMAP_F_SHARED which can be set for any data that goes into the COW Maybe we should rename it then? IOMAP_F_OUT_OF_PLACE_WRITE. Yuck. IOMAP_F_ELSEWHERE Not much better. Maybe just add a comment that "IOMAP_F_SHARED" really just means an out of place write (aka srcmap is not just a hole). --D > + * fork for XFS. > + */ > + if (iter->srcmap.type == IOMAP_HOLE || > + iter->srcmap.type == IOMAP_UNWRITTEN) > return length; > > do { > -- > 2.43.0 > >
On Mon, Aug 26, 2024 at 10:44:24PM -0700, Darrick J. Wong wrote: > Maybe we should rename it then? > > IOMAP_F_OUT_OF_PLACE_WRITE. > > Yuck. > > IOMAP_F_ELSEWHERE > > Not much better. Maybe just add a comment that "IOMAP_F_SHARED" really > just means an out of place write (aka srcmap is not just a hole). Heh. For writes it usually means out of place write, but for reporting it gets translated to the FIEMAP_EXTENT_SHARED flag or is used to reject swapon. And the there is black magic in DAX.
On Tue, Aug 27, 2024 at 07:47:57AM +0200, Christoph Hellwig wrote: > On Mon, Aug 26, 2024 at 10:44:24PM -0700, Darrick J. Wong wrote: > > Maybe we should rename it then? > > > > IOMAP_F_OUT_OF_PLACE_WRITE. > > > > Yuck. > > > > IOMAP_F_ELSEWHERE > > > > Not much better. Maybe just add a comment that "IOMAP_F_SHARED" really > > just means an out of place write (aka srcmap is not just a hole). > > Heh. > > For writes it usually means out of place write, but for reporting > it gets translated to the FIEMAP_EXTENT_SHARED flag or is used to > reject swapon. And the there is black magic in DAX. Hee hee. Yeah, let's leave IOMAP_F_SHARED alone; an out of place write can be detected by iter->srcmap.type != HOLE. The patch looks fine so Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D
On Tue, Aug 27, 2024 at 09:21:49AM -0700, Darrick J. Wong wrote: > > For writes it usually means out of place write, but for reporting > > it gets translated to the FIEMAP_EXTENT_SHARED flag or is used to > > reject swapon. And the there is black magic in DAX. > > Hee hee. Yeah, let's leave IOMAP_F_SHARED alone; an out of place write > can be detected by iter->srcmap.type != HOLE. I can probably come up with a comment that includes the COWextsize hints in the definition of out of place writes and we should be all fine..
On Wed, Aug 28, 2024 at 06:49:29AM +0200, Christoph Hellwig wrote: > On Tue, Aug 27, 2024 at 09:21:49AM -0700, Darrick J. Wong wrote: > > > For writes it usually means out of place write, but for reporting > > > it gets translated to the FIEMAP_EXTENT_SHARED flag or is used to > > > reject swapon. And the there is black magic in DAX. > > > > Hee hee. Yeah, let's leave IOMAP_F_SHARED alone; an out of place write > > can be detected by iter->srcmap.type != HOLE. out of place write => "OOP" but that acronym is already taken static inline bool iomap_write_oops(const struct iomap_iter *i) { /* ... i wrote it again */ return i->srcmap.type != HOLE; } <with apologies to the artist I just insulted> jk > I can probably come up with a comment that includes the COWextsize hints > in the definition of out of place writes and we should be all fine.. Sounds good to me. --D
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index 69a931de1979b9..737a005082e035 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -1337,16 +1337,25 @@ EXPORT_SYMBOL_GPL(iomap_file_buffered_write_punch_delalloc); static loff_t iomap_unshare_iter(struct iomap_iter *iter) { struct iomap *iomap = &iter->iomap; - const struct iomap *srcmap = iomap_iter_srcmap(iter); loff_t pos = iter->pos; loff_t length = iomap_length(iter); loff_t written = 0; - /* don't bother with blocks that are not shared to start with */ + /* Don't bother with blocks that are not shared to start with. */ if (!(iomap->flags & IOMAP_F_SHARED)) return length; - /* don't bother with holes or unwritten extents */ - if (srcmap->type == IOMAP_HOLE || srcmap->type == IOMAP_UNWRITTEN) + + /* + * Don't bother with holes or unwritten extents. + * + * Note that we use srcmap directly instead of iomap_iter_srcmap as + * unsharing requires providing a separate source map, and the presence + * of one is a good indicator that unsharing is needed, unlike + * IOMAP_F_SHARED which can be set for any data that goes into the COW + * fork for XFS. + */ + if (iter->srcmap.type == IOMAP_HOLE || + iter->srcmap.type == IOMAP_UNWRITTEN) return length; do {
Currently iomap_unshare_iter relies on the IOMAP_F_SHARED flag to detect blocks to unshare. This is reasonable, but IOMAP_F_SHARED is also useful for the file system to do internal book keeping for out of place writes. XFS used to that, until it got removed in commit 72a048c1056a ("xfs: only set IOMAP_F_SHARED when providing a srcmap to a write") because unshare for incorrectly unshare such blocks. Add an extra safeguard by checking the explicitly provided srcmap instead of the fallback to the iomap for valid data, as that catches the case where we'd just copy from the same place we'd write to easily, allowing to reinstate setting IOMAP_F_SHARED for all XFS writes that go to the COW fork. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/iomap/buffered-io.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)