Re: [PATCHv9 0/6] iomap: Add support for per-block dirty state to improve write performance

[Ritesh Harjani (IBM) <ritesh.list@gmail.com>]

Ritesh Harjani (IBM) <ritesh.list@gmail.com> writes:

> "Ritesh Harjani (IBM)" <ritesh.list@gmail.com> writes:
>
> Hello All,
>
> So I gave some thoughts about function naming and I guess the reason we
> are ping ponging between the different namings is that I am not able to
> properly articulate the reasoning behind, why we chose iomap_ifs_**.
>
> Here is my attempt to convince everyone....
>
> In one of the previous versions of the patchsets, Christoph opposed the
> idea of naming these functions with iop_** because he wanted iomap_ as a
> prefix in all of these function names. Now that I gave more thought to it,
> I too agree that we should have iomap_ as prefix in these APIs. Because
> - fs/iomap/buffered-io.c follows that style for all other functions.
> - It then also becomes easy in finding function names using ctags and
>   in doing grep or fuzzy searches.
>
> Now why "ifs" in the naming because we are abbrevating iomap_folio_state
> as "ifs". And since we are passing ifs as an argument in these functions
> and operating upon it, hence the naming of all of these functions should
> go as iomap_ifs_**.
>
> Now if I am reading all of the emails correctly, none of the reviewers have
> any strong objections with iomap_ifs_** naming style. Some of us just
> started with nitpicking, but there are no strong objections, I feel.
> Also I do think iomap_ifs_** naming is completely apt for these
> functional changes.
>
> So if no one has any strong objections, could we please continue with
> iomap_ifs_** naming itself.
>
> In case if someone does oppose strongly, I would humbly request to please
> also convince the rest of the reviewers on why your function naming
> should be chosen by giving proper reasoning (like above).
> I can definitely help with making the required changes and testing them.
>
> Does this look good and sound fair for the function naming part?

Hello All,

Hope I didn't take too long to respond to my previous email.
I had a discussion with Darrick and he convinced me with -

- ifs_ naming style will be much shorter and hence preferred.
- ifs_ already means iomap_folio_state_** v/s iomap_fs_** means
  iomap_iomap_folio_state... makes iomap_ifs_ naming awkward.
- All of these functions are anyway local and static to
  fs/iomap/buffered-io.c file so it is ok even if we have a shorter
  names like ifs_alloc()/ifs_free() etc.

Hence I have decided to go with ifs_ options for v10 which Darrick (including few others) agreed to here [1]

[1]: https://lore.kernel.org/linux-xfs/87h6r8wyxa.fsf@doe.com/T/#m7c061e634243f835ecddfb642bbfb091a9227ec7

-ritesh


>
> If everyone is convinced with iomap_ifs_** naming, then I will go ahead
> and work on the rest of the review comments.
>
> Thanks a lot for all the great review!
> -ritesh