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

[Jens Axboe <axboe@kernel.dk>]

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