Re: [PATCH 4/5] iomap: add struct iomap_data

Jens Axboe
2019-12-13T20:47:41Z

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@kernel.dk>
>
> 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