On 12/13/19 1:32 PM, Darrick J. Wong wrote: > On Thu, Dec 12, 2019 at 12:01:32PM -0700, Jens Axboe wrote: >> We pass a lot of arguments to iomap_apply(), and subsequently to the >> actors that it calls. In preparation for adding one more argument, >> switch them to using a struct iomap_data instead. The actor gets a const >> version of that, they are not supposed to change anything in it. >> >> Signed-off-by: Jens Axboe <axboe@xxxxxxxxx> > > Looks good, only a couple of questions... Thanks! >> fs/dax.c | 25 +++-- >> fs/iomap/apply.c | 26 +++--- >> fs/iomap/buffered-io.c | 202 +++++++++++++++++++++++++---------------- >> fs/iomap/direct-io.c | 57 +++++++----- >> fs/iomap/fiemap.c | 48 ++++++---- >> fs/iomap/seek.c | 64 ++++++++----- >> fs/iomap/swapfile.c | 27 +++--- >> include/linux/iomap.h | 15 ++- >> 8 files changed, 278 insertions(+), 186 deletions(-) >> >> diff --git a/fs/dax.c b/fs/dax.c >> index 1f1f0201cad1..d1c32dbbdf24 100644 >> --- a/fs/dax.c >> +++ b/fs/dax.c >> @@ -1090,13 +1090,16 @@ int __dax_zero_page_range(struct block_device *bdev, >> EXPORT_SYMBOL_GPL(__dax_zero_page_range); >> >> static loff_t >> -dax_iomap_actor(struct inode *inode, loff_t pos, loff_t length, void *data, >> - struct iomap *iomap, struct iomap *srcmap) >> +dax_iomap_actor(const struct iomap_data *data, struct iomap *iomap, > > I wonder, is 'struct iomap_ctx' a better name for the context structure? Yeah I think you are right, does seem like a better fit. I'll rename it for the next version. >> @@ -43,17 +44,18 @@ iomap_apply(struct inode *inode, loff_t pos, loff_t length, unsigned flags, >> * expose transient stale data. If the reserve fails, we can safely >> * back out at this point as there is nothing to undo. >> */ >> - ret = ops->iomap_begin(inode, pos, length, flags, &iomap, &srcmap); >> + ret = ops->iomap_begin(data->inode, data->pos, data->len, data->flags, >> + &iomap, &srcmap); > > ...and second, what do people think about about passing "const struct > iomap_ctx *ctx" to iomap_begin and iomap_end to reduce the argument > counts there too? > > (That's definitely a separate patch though, and I might just do that on > my own if nobody beats me to it...) To be honest, I just did what I needed, but I do think it's worth pursuing in general. The argument clutter is real. -- Jens Axboe
