| Message ID | 20210719103520.495450-18-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [01/27] iomap: fix a trivial comment typo in trace.h | expand |
On Mon, Jul 19, 2021 at 12:35:10PM +0200, Christoph Hellwig wrote: > Rewrite iomap_seek_hole to use iomap_iter. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/iomap/seek.c | 46 +++++++++++++++++++++++----------------------- > 1 file changed, 23 insertions(+), 23 deletions(-) > > diff --git a/fs/iomap/seek.c b/fs/iomap/seek.c > index ce6fb810854fec..7d6ed9af925e96 100644 > --- a/fs/iomap/seek.c > +++ b/fs/iomap/seek.c > @@ -1,7 +1,7 @@ > // SPDX-License-Identifier: GPL-2.0 > /* > * Copyright (C) 2017 Red Hat, Inc. > - * Copyright (c) 2018 Christoph Hellwig. > + * Copyright (c) 2018-2021 Christoph Hellwig. > */ > #include <linux/module.h> > #include <linux/compiler.h> > @@ -10,21 +10,19 @@ > #include <linux/pagemap.h> > #include <linux/pagevec.h> > > -static loff_t > -iomap_seek_hole_actor(struct inode *inode, loff_t start, loff_t length, > - void *data, struct iomap *iomap, struct iomap *srcmap) > +static loff_t iomap_seek_hole_iter(const struct iomap_iter *iter, loff_t *pos) /me wonders if @pos should be named hole_pos (here and in the caller) to make it a little easier to read... > { > - loff_t offset = start; > + loff_t length = iomap_length(iter); > > - switch (iomap->type) { > + switch (iter->iomap.type) { > case IOMAP_UNWRITTEN: > - offset = mapping_seek_hole_data(inode->i_mapping, start, > - start + length, SEEK_HOLE); > - if (offset == start + length) > + *pos = mapping_seek_hole_data(iter->inode->i_mapping, > + iter->pos, iter->pos + length, SEEK_HOLE); > + if (*pos == iter->pos + length) > return length; > - fallthrough; > + return 0; > case IOMAP_HOLE: > - *(loff_t *)data = offset; > + *pos = iter->pos; > return 0; > default: > return length; > @@ -35,23 +33,25 @@ loff_t > iomap_seek_hole(struct inode *inode, loff_t offset, const struct iomap_ops *ops) > { > loff_t size = i_size_read(inode); > - loff_t ret; > + struct iomap_iter iter = { > + .inode = inode, > + .pos = offset, > + .flags = IOMAP_REPORT, > + }; > + int ret; > > /* Nothing to be found before or beyond the end of the file. */ > if (offset < 0 || offset >= size) > return -ENXIO; > > - while (offset < size) { > - ret = iomap_apply(inode, offset, size - offset, IOMAP_REPORT, > - ops, &offset, iomap_seek_hole_actor); > - if (ret < 0) > - return ret; > - if (ret == 0) > - break; > - offset += ret; > - } > - > - return offset; > + iter.len = size - offset; > + while ((ret = iomap_iter(&iter, ops)) > 0) > + iter.processed = iomap_seek_hole_iter(&iter, &offset); > + if (ret < 0) > + return ret; > + if (iter.len) > + return offset; ...because what we're really saying here is that if seek_hole_iter found a hole (and returned zero, thereby terminating the loop before iter.len could reach zero), we want to return the position of the hole. > + return size; Not sure why we return size here...? Oh, because there's an implicit hole at EOF, so we return i_size. Uh, does this do the right thing if ->iomap_begin returns posteof mappings? I don't see anything in iomap_iter_advance that would stop iteration at EOF. --D > } > EXPORT_SYMBOL_GPL(iomap_seek_hole); > > -- > 2.30.2 >
On Mon, Jul 19, 2021 at 10:22:47AM -0700, Darrick J. Wong wrote: > > -static loff_t > > -iomap_seek_hole_actor(struct inode *inode, loff_t start, loff_t length, > > - void *data, struct iomap *iomap, struct iomap *srcmap) > > +static loff_t iomap_seek_hole_iter(const struct iomap_iter *iter, loff_t *pos) > > /me wonders if @pos should be named hole_pos (here and in the caller) to > make it a little easier to read... Sure. > ...because what we're really saying here is that if seek_hole_iter found > a hole (and returned zero, thereby terminating the loop before iter.len > could reach zero), we want to return the position of the hole. Yes. > > + return size; > > Not sure why we return size here...? Oh, because there's an implicit > hole at EOF, so we return i_size. Uh, does this do the right thing if > ->iomap_begin returns posteof mappings? I don't see anything in > iomap_iter_advance that would stop iteration at EOF. Nothing in ->iomap_begin checks that, iomap_seek_hole initializes iter.len so that it stops at EOF.
On Mon, Jul 26, 2021 at 10:22:36AM +0200, Christoph Hellwig wrote: > On Mon, Jul 19, 2021 at 10:22:47AM -0700, Darrick J. Wong wrote: > > > -static loff_t > > > -iomap_seek_hole_actor(struct inode *inode, loff_t start, loff_t length, > > > - void *data, struct iomap *iomap, struct iomap *srcmap) > > > +static loff_t iomap_seek_hole_iter(const struct iomap_iter *iter, loff_t *pos) > > > > /me wonders if @pos should be named hole_pos (here and in the caller) to > > make it a little easier to read... > > Sure. > > > ...because what we're really saying here is that if seek_hole_iter found > > a hole (and returned zero, thereby terminating the loop before iter.len > > could reach zero), we want to return the position of the hole. > > Yes. > > > > + return size; > > > > Not sure why we return size here...? Oh, because there's an implicit > > hole at EOF, so we return i_size. Uh, does this do the right thing if > > ->iomap_begin returns posteof mappings? I don't see anything in > > iomap_iter_advance that would stop iteration at EOF. > > Nothing in ->iomap_begin checks that, iomap_seek_hole initializes > iter.len so that it stops at EOF. Oh, right. Sorry, I forgot that. :( --D
diff --git a/fs/iomap/seek.c b/fs/iomap/seek.c index ce6fb810854fec..7d6ed9af925e96 100644 --- a/fs/iomap/seek.c +++ b/fs/iomap/seek.c @@ -1,7 +1,7 @@ // SPDX-License-Identifier: GPL-2.0 /* * Copyright (C) 2017 Red Hat, Inc. - * Copyright (c) 2018 Christoph Hellwig. + * Copyright (c) 2018-2021 Christoph Hellwig. */ #include <linux/module.h> #include <linux/compiler.h> @@ -10,21 +10,19 @@ #include <linux/pagemap.h> #include <linux/pagevec.h> -static loff_t -iomap_seek_hole_actor(struct inode *inode, loff_t start, loff_t length, - void *data, struct iomap *iomap, struct iomap *srcmap) +static loff_t iomap_seek_hole_iter(const struct iomap_iter *iter, loff_t *pos) { - loff_t offset = start; + loff_t length = iomap_length(iter); - switch (iomap->type) { + switch (iter->iomap.type) { case IOMAP_UNWRITTEN: - offset = mapping_seek_hole_data(inode->i_mapping, start, - start + length, SEEK_HOLE); - if (offset == start + length) + *pos = mapping_seek_hole_data(iter->inode->i_mapping, + iter->pos, iter->pos + length, SEEK_HOLE); + if (*pos == iter->pos + length) return length; - fallthrough; + return 0; case IOMAP_HOLE: - *(loff_t *)data = offset; + *pos = iter->pos; return 0; default: return length; @@ -35,23 +33,25 @@ loff_t iomap_seek_hole(struct inode *inode, loff_t offset, const struct iomap_ops *ops) { loff_t size = i_size_read(inode); - loff_t ret; + struct iomap_iter iter = { + .inode = inode, + .pos = offset, + .flags = IOMAP_REPORT, + }; + int ret; /* Nothing to be found before or beyond the end of the file. */ if (offset < 0 || offset >= size) return -ENXIO; - while (offset < size) { - ret = iomap_apply(inode, offset, size - offset, IOMAP_REPORT, - ops, &offset, iomap_seek_hole_actor); - if (ret < 0) - return ret; - if (ret == 0) - break; - offset += ret; - } - - return offset; + iter.len = size - offset; + while ((ret = iomap_iter(&iter, ops)) > 0) + iter.processed = iomap_seek_hole_iter(&iter, &offset); + if (ret < 0) + return ret; + if (iter.len) + return offset; + return size; } EXPORT_SYMBOL_GPL(iomap_seek_hole);
Rewrite iomap_seek_hole to use iomap_iter. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/iomap/seek.c | 46 +++++++++++++++++++++++----------------------- 1 file changed, 23 insertions(+), 23 deletions(-)