| Message ID | 20210719103520.495450-17-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/27] iomap: fix a trivial comment typo in trace.h | expand |
On Mon, Jul 19, 2021 at 12:35:09PM +0200, Christoph Hellwig wrote: > Rewrite the ->bmap implementation based on iomap_iter. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/iomap/fiemap.c | 31 +++++++++++++------------------ > 1 file changed, 13 insertions(+), 18 deletions(-) > > diff --git a/fs/iomap/fiemap.c b/fs/iomap/fiemap.c > index acad09a8c188df..60daadba16c149 100644 > --- a/fs/iomap/fiemap.c > +++ b/fs/iomap/fiemap.c > @@ -92,35 +92,30 @@ int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi, > } > EXPORT_SYMBOL_GPL(iomap_fiemap); > > -static loff_t > -iomap_bmap_actor(struct inode *inode, loff_t pos, loff_t length, > - void *data, struct iomap *iomap, struct iomap *srcmap) > -{ > - sector_t *bno = data, addr; > - > - if (iomap->type == IOMAP_MAPPED) { > - addr = (pos - iomap->offset + iomap->addr) >> inode->i_blkbits; > - *bno = addr; > - } > - return 0; > -} > - > /* legacy ->bmap interface. 0 is the error return (!) */ > sector_t > iomap_bmap(struct address_space *mapping, sector_t bno, > const struct iomap_ops *ops) > { > - struct inode *inode = mapping->host; > - loff_t pos = bno << inode->i_blkbits; > - unsigned blocksize = i_blocksize(inode); > + struct iomap_iter iter = { > + .inode = mapping->host, > + .pos = (loff_t)bno << mapping->host->i_blkbits, > + .len = i_blocksize(mapping->host), > + .flags = IOMAP_REPORT, > + }; > int ret; > > if (filemap_write_and_wait(mapping)) > return 0; > > bno = 0; > - ret = iomap_apply(inode, pos, blocksize, 0, ops, &bno, > - iomap_bmap_actor); > + while ((ret = iomap_iter(&iter, ops)) > 0) { > + if (iter.iomap.type != IOMAP_MAPPED) > + continue; There isn't a mapped extent, so return 0 here, right? --D > + bno = (iter.pos - iter.iomap.offset + iter.iomap.addr) >> > + mapping->host->i_blkbits; > + } > + > if (ret) > return 0; > return bno; > -- > 2.30.2 >
On Mon, Jul 19, 2021 at 10:05:45AM -0700, Darrick J. Wong wrote: > > bno = 0; > > - ret = iomap_apply(inode, pos, blocksize, 0, ops, &bno, > > - iomap_bmap_actor); > > + while ((ret = iomap_iter(&iter, ops)) > 0) { > > + if (iter.iomap.type != IOMAP_MAPPED) > > + continue; > > There isn't a mapped extent, so return 0 here, right? We can't just return 0, we always need the final iomap_iter() call to clean up in case a ->iomap_end method is supplied. No for bmap having and needing one is rather theoretical, but people will copy and paste that once we start breaking the rules.
On Mon, Jul 26, 2021 at 10:19:42AM +0200, Christoph Hellwig wrote: > On Mon, Jul 19, 2021 at 10:05:45AM -0700, Darrick J. Wong wrote: > > > bno = 0; > > > - ret = iomap_apply(inode, pos, blocksize, 0, ops, &bno, > > > - iomap_bmap_actor); > > > + while ((ret = iomap_iter(&iter, ops)) > 0) { > > > + if (iter.iomap.type != IOMAP_MAPPED) > > > + continue; > > > > There isn't a mapped extent, so return 0 here, right? > > We can't just return 0, we always need the final iomap_iter() call > to clean up in case a ->iomap_end method is supplied. No for bmap > having and needing one is rather theoretical, but people will copy > and paste that once we start breaking the rules. Oh, right, I forgot that someone might want to ->iomap_end. The "continue" works because we only asked for one block, therefore we know that we'll never get to the loop body a second time; and we ignore iter.processed, which also means we never revisit the loop body. This "continue without setting iter.processed to break out of loop" pattern is a rather indirect subtlety, since C programmers are taught that they can break out of a loop using break;. This new iomap_iter pattern fubars that longstanding language feature, and the language around it is soft: > /** > * iomap_iter - iterate over a ranges in a file > * @iter: iteration structue > * @ops: iomap ops provided by the file system > * > * Iterate over file system provided contiguous ranges of blocks with the same > * state. Should be called in a loop that continues as long as this function > * returns a positive value. If 0 or a negative value is returned the caller > * should break out of the loop - a negative value is an error either from the > * file system or from the last iteration stored in @iter.copied. > */ The documentation needs to be much more explicit about the fact that you cannot "break;" your way out of an iomap_iter loop. I think the comment should be rewritten along these lines: "Iterate over filesystem-provided space mappings for the provided file range. This function handles cleanup of resources acquired for iteration when the filesystem indicates there are no more space mappings, which means that this function must be called in a loop that continues as long it returns a positive value. If 0 or a negative value is returned, the caller must not return to the loop body. Within a loop body, there are two ways to break out of the loop body: leave @iter.processed unchanged, or set it to the usual negative errno." Hm. What if we provide an explicit loop break function? That would be clear overkill for bmap, but somebody else wanting to break out of a more complex loop body ought to be able to say "break" to do that, not "continue with subtleties". static inline int iomap_iter_break(struct iomap_iter *iter, int ret) { int ret2; if (!iter->iomap.length || !ops->iomap_end) return ret; ret2 = ops->iomap_end(iter->inode, iter->pos, iomap_length(iter), 0, iter->flags, &iter->iomap); return ret ? ret : ret2; } And then then theoretical loop body becomes: while ((ret = iomap_iter(&iter, ops)) > 0) { if (iter.iomap.type != WHAT_I_WANT) { ret = iomap_iter_break(&iter, 0); break; } <large blob of code here> ret = vfs_do_some_risky_thing(...); if (ret) { ret = iomap_iter_break(&iter, ret); break; } <more loop body here> iter.processed = iter.iomap.length; } return ret; Clunky, for sure, but at least we still get to use break as the language designers intended. --D
On Mon, Jul 26, 2021 at 09:39:22AM -0700, Darrick J. Wong wrote: > The documentation needs to be much more explicit about the fact that you > cannot "break;" your way out of an iomap_iter loop. I think the comment > should be rewritten along these lines: > > "Iterate over filesystem-provided space mappings for the provided file > range. This function handles cleanup of resources acquired for > iteration when the filesystem indicates there are no more space > mappings, which means that this function must be called in a loop that > continues as long it returns a positive value. If 0 or a negative value > is returned, the caller must not return to the loop body. Within a loop > body, there are two ways to break out of the loop body: leave > @iter.processed unchanged, or set it to the usual negative errno." > > Hm. Yes, I'll update the documentation. > Clunky, for sure, but at least we still get to use break as the language > designers intended. I can't see any advantage there over just proper documentation. If you are totally attached to a working break we might have to come up with a nasty for_each macro that ensures we have a final iomap_apply, but I doubt it is worth the effort.
On Tue, Jul 27, 2021 at 08:31:38AM +0200, Christoph Hellwig wrote: > On Mon, Jul 26, 2021 at 09:39:22AM -0700, Darrick J. Wong wrote: > > The documentation needs to be much more explicit about the fact that you > > cannot "break;" your way out of an iomap_iter loop. I think the comment > > should be rewritten along these lines: > > > > "Iterate over filesystem-provided space mappings for the provided file > > range. This function handles cleanup of resources acquired for > > iteration when the filesystem indicates there are no more space > > mappings, which means that this function must be called in a loop that > > continues as long it returns a positive value. If 0 or a negative value > > is returned, the caller must not return to the loop body. Within a loop > > body, there are two ways to break out of the loop body: leave > > @iter.processed unchanged, or set it to the usual negative errno." > > > > Hm. > > Yes, I'll update the documentation. Ok, thanks! > > Clunky, for sure, but at least we still get to use break as the language > > designers intended. > > I can't see any advantage there over just proper documentation. If you > are totally attached to a working break we might have to come up with > a nasty for_each macro that ensures we have a final iomap_apply, but I > doubt it is worth the effort. I was pushing the explicit _break() function as a means to avoid an even fuglier loop macro. --D
diff --git a/fs/iomap/fiemap.c b/fs/iomap/fiemap.c index acad09a8c188df..60daadba16c149 100644 --- a/fs/iomap/fiemap.c +++ b/fs/iomap/fiemap.c @@ -92,35 +92,30 @@ int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi, } EXPORT_SYMBOL_GPL(iomap_fiemap); -static loff_t -iomap_bmap_actor(struct inode *inode, loff_t pos, loff_t length, - void *data, struct iomap *iomap, struct iomap *srcmap) -{ - sector_t *bno = data, addr; - - if (iomap->type == IOMAP_MAPPED) { - addr = (pos - iomap->offset + iomap->addr) >> inode->i_blkbits; - *bno = addr; - } - return 0; -} - /* legacy ->bmap interface. 0 is the error return (!) */ sector_t iomap_bmap(struct address_space *mapping, sector_t bno, const struct iomap_ops *ops) { - struct inode *inode = mapping->host; - loff_t pos = bno << inode->i_blkbits; - unsigned blocksize = i_blocksize(inode); + struct iomap_iter iter = { + .inode = mapping->host, + .pos = (loff_t)bno << mapping->host->i_blkbits, + .len = i_blocksize(mapping->host), + .flags = IOMAP_REPORT, + }; int ret; if (filemap_write_and_wait(mapping)) return 0; bno = 0; - ret = iomap_apply(inode, pos, blocksize, 0, ops, &bno, - iomap_bmap_actor); + while ((ret = iomap_iter(&iter, ops)) > 0) { + if (iter.iomap.type != IOMAP_MAPPED) + continue; + bno = (iter.pos - iter.iomap.offset + iter.iomap.addr) >> + mapping->host->i_blkbits; + } + if (ret) return 0; return bno;
Rewrite the ->bmap implementation based on iomap_iter. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/iomap/fiemap.c | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-)