| Message ID | cover.1686395560.git.ritesh.list@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | iomap: Add support for per-block dirty state to improve write performance | expand |
"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? 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
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
Hello All, Please find PATCHv9 which adds per-block dirty tracking to iomap. As discussed earlier this is required to improve write performance and reduce write amplification for cases where either blocksize is less than pagesize (such as Power platform with 64k pagesize) or when we have a large folio (such as xfs which currently supports large folio). v7/v8 -> v9 ============ 1. Splitted the renaming & refactoring changes into different patchsets. (Patch-1 & Patch-2) 2. Addressed review comments from everyone in v9. 3. Fixed a punch-out bug pointed out by Darrick in Patch-6. 4. Included iomap_ifs_calc_range() function suggested by Christoph in Patch-6. Testing ========= I have tested v9 on:- - Power with 4k blocksize -g auto - x86 with 1k and 1k_adv with -g auto - arm64 with 4k blocksize and 64k pagesize with 4k quick - also tested gfs2 with minimal local config (-O -b 1024 -p lock_nolock) - unit tested failed punch-out operation with "-f" support to pwrite in xfs_io. I haven't observed any new testcase failures in any of my testing so far. Thanks everyone for helping with reviews and suggestions. Please do let me know if there are any further review comments on this one. <Perf data copy paste from previous version> ============================================= Performance testing of below fio workload reveals ~16x performance improvement using nvme with XFS (4k blocksize) on Power (64K pagesize) FIO reported write bw scores improved from around ~28 MBps to ~452 MBps. 1. <test_randwrite.fio> [global] ioengine=psync rw=randwrite overwrite=1 pre_read=1 direct=0 bs=4k size=1G dir=./ numjobs=8 fdatasync=1 runtime=60 iodepth=64 group_reporting=1 [fio-run] 2. Also our internal performance team reported that this patch improves their database workload performance by around ~83% (with XFS on Power) Ritesh Harjani (IBM) (6): iomap: Rename iomap_page to iomap_folio_state and others iomap: Drop ifs argument from iomap_set_range_uptodate() iomap: Add some uptodate state handling helpers for ifs state bitmap iomap: Refactor iomap_write_delalloc_punch() function out iomap: Allocate ifs in ->write_begin() early iomap: Add per-block dirty state tracking to improve performance fs/gfs2/aops.c | 2 +- fs/iomap/buffered-io.c | 394 +++++++++++++++++++++++++++++------------ fs/xfs/xfs_aops.c | 2 +- fs/zonefs/file.c | 2 +- include/linux/iomap.h | 1 + 5 files changed, 282 insertions(+), 119 deletions(-) -- 2.40.1