| Message ID | 20241002150213.GC21853@frogsfrogsfrogs (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [1/2] iomap: don't bother unsharing delalloc extents | expand |
On Wed, Oct 02, 2024 at 08:02:13AM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > File contents can only be shared (i.e. reflinked) below EOF, so it makes > no sense to try to unshare ranges beyond EOF. Constrain the file range > parameters here so that we don't have to do that in the callers. Looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
On Wed, Oct 02, 2024 at 08:02:13AM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > File contents can only be shared (i.e. reflinked) below EOF, so it makes > no sense to try to unshare ranges beyond EOF. Constrain the file range > parameters here so that we don't have to do that in the callers. > > Fixes: 5f4e5752a8a3 ("fs: add iomap_file_dirty") > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > --- > fs/dax.c | 6 +++++- > fs/iomap/buffered-io.c | 6 +++++- > 2 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/fs/dax.c b/fs/dax.c > index becb4a6920c6a..c62acd2812f8d 100644 > --- a/fs/dax.c > +++ b/fs/dax.c > @@ -1305,11 +1305,15 @@ int dax_file_unshare(struct inode *inode, loff_t pos, loff_t len, > struct iomap_iter iter = { > .inode = inode, > .pos = pos, > - .len = len, > .flags = IOMAP_WRITE | IOMAP_UNSHARE | IOMAP_DAX, > }; > + loff_t size = i_size_read(inode); > int ret; > > + if (pos < 0 || pos >= size) > + return 0; > + > + iter.len = min(len, size - pos); > while ((ret = iomap_iter(&iter, ops)) > 0) > iter.processed = dax_unshare_iter(&iter); > return ret; > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index c1c559e0cc07c..78ebd265f4259 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -1375,11 +1375,15 @@ iomap_file_unshare(struct inode *inode, loff_t pos, loff_t len, > struct iomap_iter iter = { > .inode = inode, > .pos = pos, > - .len = len, > .flags = IOMAP_WRITE | IOMAP_UNSHARE, > }; > + loff_t size = i_size_read(inode); > int ret; > > + if (pos < 0 || pos >= size) > + return 0; > + > + iter.len = min(len, size - pos); > while ((ret = iomap_iter(&iter, ops)) > 0) > iter.processed = iomap_unshare_iter(&iter); > return ret; > Heh. This was pretty much my local fix when I was testing fsx unshare range, so LGTM. Apologies, I probably should have just sent it out. It just seemed like Julian was 90% there, but then review went off the rails and I guess I lost interest. Anyways, thanks for the fix: Reviewed-by: Brian Foster <bfoster@redhat.com>
On Wed, 2024-10-02 at 08:02 -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > File contents can only be shared (i.e. reflinked) below EOF, so it makes > no sense to try to unshare ranges beyond EOF. Constrain the file range > parameters here so that we don't have to do that in the callers. > > Fixes: 5f4e5752a8a3 ("fs: add iomap_file_dirty") > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > --- > fs/dax.c | 6 +++++- > fs/iomap/buffered-io.c | 6 +++++- > 2 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/fs/dax.c b/fs/dax.c > index becb4a6920c6a..c62acd2812f8d 100644 > --- a/fs/dax.c > +++ b/fs/dax.c > @@ -1305,11 +1305,15 @@ int dax_file_unshare(struct inode *inode, loff_t > pos, loff_t len, > struct iomap_iter iter = { > .inode = inode, > .pos = pos, > - .len = len, > .flags = IOMAP_WRITE | IOMAP_UNSHARE | > IOMAP_DAX, > }; > + loff_t size = i_size_read(inode); > int ret; > > + if (pos < 0 || pos >= size) > + return 0; > + > + iter.len = min(len, size - pos); > while ((ret = iomap_iter(&iter, ops)) > 0) > iter.processed = dax_unshare_iter(&iter); > return ret; > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index c1c559e0cc07c..78ebd265f4259 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -1375,11 +1375,15 @@ iomap_file_unshare(struct inode *inode, loff_t > pos, loff_t len, > struct iomap_iter iter = { > .inode = inode, > .pos = pos, > - .len = len, > .flags = IOMAP_WRITE | IOMAP_UNSHARE, > }; > + loff_t size = i_size_read(inode); > int ret; > > + if (pos < 0 || pos >= size) > + return 0; > + > + iter.len = min(len, size - pos); > while ((ret = iomap_iter(&iter, ops)) > 0) > iter.processed = iomap_unshare_iter(&iter); > return ret; Sorry I didn’t update the patch sooner—I was planning to get to it after wrapping up my week-long vacation... Anyway, thanks for the patch, it looks great! Thanks,
diff --git a/fs/dax.c b/fs/dax.c index becb4a6920c6a..c62acd2812f8d 100644 --- a/fs/dax.c +++ b/fs/dax.c @@ -1305,11 +1305,15 @@ int dax_file_unshare(struct inode *inode, loff_t pos, loff_t len, struct iomap_iter iter = { .inode = inode, .pos = pos, - .len = len, .flags = IOMAP_WRITE | IOMAP_UNSHARE | IOMAP_DAX, }; + loff_t size = i_size_read(inode); int ret; + if (pos < 0 || pos >= size) + return 0; + + iter.len = min(len, size - pos); while ((ret = iomap_iter(&iter, ops)) > 0) iter.processed = dax_unshare_iter(&iter); return ret; diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index c1c559e0cc07c..78ebd265f4259 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -1375,11 +1375,15 @@ iomap_file_unshare(struct inode *inode, loff_t pos, loff_t len, struct iomap_iter iter = { .inode = inode, .pos = pos, - .len = len, .flags = IOMAP_WRITE | IOMAP_UNSHARE, }; + loff_t size = i_size_read(inode); int ret; + if (pos < 0 || pos >= size) + return 0; + + iter.len = min(len, size - pos); while ((ret = iomap_iter(&iter, ops)) > 0) iter.processed = iomap_unshare_iter(&iter); return ret;