f2fs-tools: correct some confused desc about unit

[PATCH] f2fs-tools: correct some confused desc about unit

Posted by Zhiguo Niu 1 month ago

F2FS_BLKSIZE may be 4KB, 16KB, so use F2FS_BLKSIZE to replace
some hardcode desc about unit in some f2fs_io cmd, also
adjust "-c" parameters in mkfs man, to be consistent with
commit c35fa8cd75ac ("mkfs.f2fs: change -c option description").

Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
---
 man/mkfs.f2fs.8         | 2 +-
 tools/f2fs_io/f2fs_io.c | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/man/mkfs.f2fs.8 b/man/mkfs.f2fs.8
index de885be..8b3b0cc 100644
--- a/man/mkfs.f2fs.8
+++ b/man/mkfs.f2fs.8
@@ -122,7 +122,7 @@ block size matches the page size.
 The default value is 4096.
 .TP
 .BI \-c " device-list"
-Build f2fs with these additional comma separated devices, so that the user can
+Build f2fs with these additional devices, so that the user can
 see all the devices as one big volume.
 Supports up to 7 devices except meta device.
 .TP
diff --git a/tools/f2fs_io/f2fs_io.c b/tools/f2fs_io/f2fs_io.c
index 95f575f..ee4ed0e 100644
--- a/tools/f2fs_io/f2fs_io.c
+++ b/tools/f2fs_io/f2fs_io.c
@@ -1013,7 +1013,7 @@ static void do_randread(int argc, char **argv, const struct cmd_desc *cmd)
 
 #define fiemap_desc "get block address in file"
 #define fiemap_help					\
-"f2fs_io fiemap [offset in 4kb] [count in 4kb] [file_path]\n\n"\
+"f2fs_io fiemap [offset in F2FS_BLKSIZE] [count in F2FS_BLKSIZE] [file_path]\n\n"\
 
 #if defined(HAVE_LINUX_FIEMAP_H) && defined(HAVE_LINUX_FS_H)
 static void do_fiemap(int argc, char **argv, const struct cmd_desc *cmd)
@@ -1617,8 +1617,8 @@ static void do_move_range(int argc, char **argv, const struct cmd_desc *cmd)
 #define gc_range_desc "trigger filesystem gc_range"
 #define gc_range_help "f2fs_io gc_range [sync_mode] [start] [length] [file_path]\n\n"\
 "  sync_mode : 0: asynchronous, 1: synchronous\n"			\
-"  start     : start offset of defragment region, unit: 4kb\n"	\
-"  length    : bytes number of defragment region, unit: 4kb\n"	\
+"  start     : start offset of defragment region, unit: F2FS_BLKSIZE\n"	\
+"  length    : bytes number of defragment region, unit: F2FS_BLKSIZE\n"	\
 
 static void do_gc_range(int argc, char **argv, const struct cmd_desc *cmd)
 {
-- 
1.9.1

Re: [PATCH] f2fs-tools: correct some confused desc about unit

Posted by Chao Yu 1 month ago

On 2024/10/24 17:28, Zhiguo Niu wrote:
> F2FS_BLKSIZE may be 4KB, 16KB, so use F2FS_BLKSIZE to replace
> some hardcode desc about unit in some f2fs_io cmd, also
> adjust "-c" parameters in mkfs man, to be consistent with
> commit c35fa8cd75ac ("mkfs.f2fs: change -c option description").
> 
> Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
> ---
>   man/mkfs.f2fs.8         | 2 +-
>   tools/f2fs_io/f2fs_io.c | 6 +++---
>   2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/man/mkfs.f2fs.8 b/man/mkfs.f2fs.8
> index de885be..8b3b0cc 100644
> --- a/man/mkfs.f2fs.8
> +++ b/man/mkfs.f2fs.8
> @@ -122,7 +122,7 @@ block size matches the page size.
>   The default value is 4096.
>   .TP
>   .BI \-c " device-list"
> -Build f2fs with these additional comma separated devices, so that the user can
> +Build f2fs with these additional devices, so that the user can
>   see all the devices as one big volume.
>   Supports up to 7 devices except meta device.
>   .TP
> diff --git a/tools/f2fs_io/f2fs_io.c b/tools/f2fs_io/f2fs_io.c
> index 95f575f..ee4ed0e 100644
> --- a/tools/f2fs_io/f2fs_io.c
> +++ b/tools/f2fs_io/f2fs_io.c
> @@ -1013,7 +1013,7 @@ static void do_randread(int argc, char **argv, const struct cmd_desc *cmd)
>   
>   #define fiemap_desc "get block address in file"
>   #define fiemap_help					\
> -"f2fs_io fiemap [offset in 4kb] [count in 4kb] [file_path]\n\n"\
> +"f2fs_io fiemap [offset in F2FS_BLKSIZE] [count in F2FS_BLKSIZE] [file_path]\n\n"\
>   
>   #if defined(HAVE_LINUX_FIEMAP_H) && defined(HAVE_LINUX_FS_H)
>   static void do_fiemap(int argc, char **argv, const struct cmd_desc *cmd)
> @@ -1617,8 +1617,8 @@ static void do_move_range(int argc, char **argv, const struct cmd_desc *cmd)
>   #define gc_range_desc "trigger filesystem gc_range"
>   #define gc_range_help "f2fs_io gc_range [sync_mode] [start] [length] [file_path]\n\n"\
>   "  sync_mode : 0: asynchronous, 1: synchronous\n"			\
> -"  start     : start offset of defragment region, unit: 4kb\n"	\
> -"  length    : bytes number of defragment region, unit: 4kb\n"	\
> +"  start     : start offset of defragment region, unit: F2FS_BLKSIZE\n"	\
> +"  length    : bytes number of defragment region, unit: F2FS_BLKSIZE\n"	\

I think we'd better unify default block size to 4096 since in most of
places in f2fs_io.c, we use 4096 as default IO/buffer size.

git grep -n "4096" tools/f2fs_io/f2fs_io.c
tools/f2fs_io/f2fs_io.c:212:    args.block_size = 4096;
tools/f2fs_io/f2fs_io.c:662:    buf_size = bs * 4096;
tools/f2fs_io/f2fs_io.c:666:    buf = aligned_xalloc(4096, buf_size);
tools/f2fs_io/f2fs_io.c:877:    buf_size = bs * 4096;
tools/f2fs_io/f2fs_io.c:881:    buf = aligned_xalloc(4096, buf_size);
tools/f2fs_io/f2fs_io.c:901:            if (posix_fadvise(fd, 0, 4096, POSIX_FADV_SEQUENTIAL) != 0)
tools/f2fs_io/f2fs_io.c:903:            if (posix_fadvise(fd, 0, 4096, POSIX_FADV_WILLNEED) != 0)
tools/f2fs_io/f2fs_io.c:979:    buf_size = bs * 4096;
tools/f2fs_io/f2fs_io.c:981:    buf = aligned_xalloc(4096, buf_size);
tools/f2fs_io/f2fs_io.c:994:    aligned_size = (u64)stbuf.st_size & ~((u64)(4096 - 1));
tools/f2fs_io/f2fs_io.c:997:    end_idx = (u64)(aligned_size - buf_size) / (u64)4096 + 1;
tools/f2fs_io/f2fs_io.c:1004:           ret = pread(fd, buf, buf_size, 4096 * idx);
tools/f2fs_io/f2fs_io.c:1222:           char *buf = aligned_xalloc(4096, 4096);
tools/f2fs_io/f2fs_io.c:1224:           while ((ret = xread(src_fd, buf, 4096)) > 0)

git grep -n "F2FS_BLKSIZE" tools/f2fs_io/f2fs_io.c
tools/f2fs_io/f2fs_io.c:1034:   start = (u64)atoi(argv[1]) * F2FS_BLKSIZE;
tools/f2fs_io/f2fs_io.c:1035:   length = (u64)atoi(argv[2]) * F2FS_BLKSIZE;
tools/f2fs_io/f2fs_io.c:1042:                           start / F2FS_BLKSIZE, length / F2FS_BLKSIZE);

We can add a new macro F2FS_DEFAULT_BLKSIZE and use it instead of magic
number and F2FS_BLKSIZE, what do you think?

Thanks,

>   
>   static void do_gc_range(int argc, char **argv, const struct cmd_desc *cmd)
>   {

Re: [PATCH] f2fs-tools: correct some confused desc about unit

Posted by Zhiguo Niu 1 month ago

Chao Yu <chao@kernel.org> 于2024年10月24日周四 18:49写道：
>
> On 2024/10/24 17:28, Zhiguo Niu wrote:
> > F2FS_BLKSIZE may be 4KB, 16KB, so use F2FS_BLKSIZE to replace
> > some hardcode desc about unit in some f2fs_io cmd, also
> > adjust "-c" parameters in mkfs man, to be consistent with
> > commit c35fa8cd75ac ("mkfs.f2fs: change -c option description").
> >
> > Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
> > ---
> >   man/mkfs.f2fs.8         | 2 +-
> >   tools/f2fs_io/f2fs_io.c | 6 +++---
> >   2 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/man/mkfs.f2fs.8 b/man/mkfs.f2fs.8
> > index de885be..8b3b0cc 100644
> > --- a/man/mkfs.f2fs.8
> > +++ b/man/mkfs.f2fs.8
> > @@ -122,7 +122,7 @@ block size matches the page size.
> >   The default value is 4096.
> >   .TP
> >   .BI \-c " device-list"
> > -Build f2fs with these additional comma separated devices, so that the user can
> > +Build f2fs with these additional devices, so that the user can
> >   see all the devices as one big volume.
> >   Supports up to 7 devices except meta device.
> >   .TP
> > diff --git a/tools/f2fs_io/f2fs_io.c b/tools/f2fs_io/f2fs_io.c
> > index 95f575f..ee4ed0e 100644
> > --- a/tools/f2fs_io/f2fs_io.c
> > +++ b/tools/f2fs_io/f2fs_io.c
> > @@ -1013,7 +1013,7 @@ static void do_randread(int argc, char **argv, const struct cmd_desc *cmd)
> >
> >   #define fiemap_desc "get block address in file"
> >   #define fiemap_help                                 \
> > -"f2fs_io fiemap [offset in 4kb] [count in 4kb] [file_path]\n\n"\
> > +"f2fs_io fiemap [offset in F2FS_BLKSIZE] [count in F2FS_BLKSIZE] [file_path]\n\n"\
> >
> >   #if defined(HAVE_LINUX_FIEMAP_H) && defined(HAVE_LINUX_FS_H)
> >   static void do_fiemap(int argc, char **argv, const struct cmd_desc *cmd)
> > @@ -1617,8 +1617,8 @@ static void do_move_range(int argc, char **argv, const struct cmd_desc *cmd)
> >   #define gc_range_desc "trigger filesystem gc_range"
> >   #define gc_range_help "f2fs_io gc_range [sync_mode] [start] [length] [file_path]\n\n"\
> >   "  sync_mode : 0: asynchronous, 1: synchronous\n"                   \
> > -"  start     : start offset of defragment region, unit: 4kb\n"       \
> > -"  length    : bytes number of defragment region, unit: 4kb\n"       \
> > +"  start     : start offset of defragment region, unit: F2FS_BLKSIZE\n"      \
> > +"  length    : bytes number of defragment region, unit: F2FS_BLKSIZE\n"      \
>
> I think we'd better unify default block size to 4096 since in most of
> places in f2fs_io.c, we use 4096 as default IO/buffer size.
>
> git grep -n "4096" tools/f2fs_io/f2fs_io.c
> tools/f2fs_io/f2fs_io.c:212:    args.block_size = 4096;
> tools/f2fs_io/f2fs_io.c:662:    buf_size = bs * 4096;
> tools/f2fs_io/f2fs_io.c:666:    buf = aligned_xalloc(4096, buf_size);
> tools/f2fs_io/f2fs_io.c:877:    buf_size = bs * 4096;
> tools/f2fs_io/f2fs_io.c:881:    buf = aligned_xalloc(4096, buf_size);
> tools/f2fs_io/f2fs_io.c:901:            if (posix_fadvise(fd, 0, 4096, POSIX_FADV_SEQUENTIAL) != 0)
> tools/f2fs_io/f2fs_io.c:903:            if (posix_fadvise(fd, 0, 4096, POSIX_FADV_WILLNEED) != 0)
> tools/f2fs_io/f2fs_io.c:979:    buf_size = bs * 4096;
> tools/f2fs_io/f2fs_io.c:981:    buf = aligned_xalloc(4096, buf_size);
> tools/f2fs_io/f2fs_io.c:994:    aligned_size = (u64)stbuf.st_size & ~((u64)(4096 - 1));
> tools/f2fs_io/f2fs_io.c:997:    end_idx = (u64)(aligned_size - buf_size) / (u64)4096 + 1;
> tools/f2fs_io/f2fs_io.c:1004:           ret = pread(fd, buf, buf_size, 4096 * idx);
> tools/f2fs_io/f2fs_io.c:1222:           char *buf = aligned_xalloc(4096, 4096);
> tools/f2fs_io/f2fs_io.c:1224:           while ((ret = xread(src_fd, buf, 4096)) > 0)
>
> git grep -n "F2FS_BLKSIZE" tools/f2fs_io/f2fs_io.c
> tools/f2fs_io/f2fs_io.c:1034:   start = (u64)atoi(argv[1]) * F2FS_BLKSIZE;
> tools/f2fs_io/f2fs_io.c:1035:   length = (u64)atoi(argv[2]) * F2FS_BLKSIZE;
> tools/f2fs_io/f2fs_io.c:1042:                           start / F2FS_BLKSIZE, length / F2FS_BLKSIZE);
>
> We can add a new macro F2FS_DEFAULT_BLKSIZE and use it instead of magic
> number and F2FS_BLKSIZE, what do you think?
Dear Chao,
It is a good  suggestions,  will update it.
now it is a little confused when I use f2fs_io fiemap in 16KB page system. ^^
thanks!
>
> Thanks,
>
> >
> >   static void do_gc_range(int argc, char **argv, const struct cmd_desc *cmd)
> >   {
>