| Message ID | 20231118020309.1894531-3-drosen@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | F2FS Tools 16K Bug Fixes | expand |
On 2023/11/18 10:03, Daniel Rosenberg wrote: > The cache is initialized during the first read, however, it requires the > block size to establish its buffer. This disables the cache until the > block size is known. Hi Daniel, How about this? It be more explicit to indicate the logic? --- fsck/mount.c | 2 ++ include/f2fs_fs.h | 3 +++ lib/libf2fs_io.c | 4 ++++ 3 files changed, 9 insertions(+) diff --git a/fsck/mount.c b/fsck/mount.c index 72516f4..4dfb996 100644 --- a/fsck/mount.c +++ b/fsck/mount.c @@ -1238,6 +1238,8 @@ int init_sb_info(struct f2fs_sb_info *sbi) MSG(0, "Info: total FS sectors = %"PRIu64" (%"PRIu64" MB)\n", total_sectors, total_sectors >> (20 - get_sb(log_sectorsize))); + + c.cache_config.blksize_initialized = true; return 0; } diff --git a/include/f2fs_fs.h b/include/f2fs_fs.h index 6df2e73..e377d48 100644 --- a/include/f2fs_fs.h +++ b/include/f2fs_fs.h @@ -460,6 +460,9 @@ typedef struct { unsigned max_hash_collision; bool dbg_en; + + /* whether blksize has been initialized */ + bool blksize_initialized; } dev_cache_config_t; /* f2fs_configration for compression used for sload.f2fs */ diff --git a/lib/libf2fs_io.c b/lib/libf2fs_io.c index 39d3777..bb77418 100644 --- a/lib/libf2fs_io.c +++ b/lib/libf2fs_io.c @@ -199,6 +199,10 @@ void dcache_init(void) { long n; + /* Must not initiate cache until block size is known */ + if (!c.cache_config.blksize_initialized) + return; + if (c.cache_config.num_cache_entry <= 0) return;
On Sun, Nov 26, 2023 at 5:42 PM Chao Yu <chao@kernel.org> wrote: > > Hi Daniel, > > How about this? It be more explicit to indicate the logic? > > --- > fsck/mount.c | 2 ++ > include/f2fs_fs.h | 3 +++ > lib/libf2fs_io.c | 4 ++++ > 3 files changed, 9 insertions(+) > > diff --git a/fsck/mount.c b/fsck/mount.c > index 72516f4..4dfb996 100644 > --- a/fsck/mount.c > +++ b/fsck/mount.c > @@ -1238,6 +1238,8 @@ int init_sb_info(struct f2fs_sb_info *sbi) > MSG(0, "Info: total FS sectors = %"PRIu64" (%"PRIu64" MB)\n", > total_sectors, total_sectors >> > (20 - get_sb(log_sectorsize))); > + > + c.cache_config.blksize_initialized = true; > return 0; > } > > diff --git a/include/f2fs_fs.h b/include/f2fs_fs.h > index 6df2e73..e377d48 100644 > --- a/include/f2fs_fs.h > +++ b/include/f2fs_fs.h > @@ -460,6 +460,9 @@ typedef struct { > unsigned max_hash_collision; > > bool dbg_en; > + > + /* whether blksize has been initialized */ > + bool blksize_initialized; > } dev_cache_config_t; > > /* f2fs_configration for compression used for sload.f2fs */ > diff --git a/lib/libf2fs_io.c b/lib/libf2fs_io.c > index 39d3777..bb77418 100644 > --- a/lib/libf2fs_io.c > +++ b/lib/libf2fs_io.c > @@ -199,6 +199,10 @@ void dcache_init(void) > { > long n; > > + /* Must not initiate cache until block size is known */ > + if (!c.cache_config.blksize_initialized) > + return; > + > if (c.cache_config.num_cache_entry <= 0) > return; > > -- > 2.40.1 > > I think that works too. I was initially going to go with code like that, but I was unsure if read/writes that aren't in the fsck path used caching as well. There's a few uses of those in the mkfs/ paths, but I don't think there's anything that sets cache_config.num_cache_entry there. I was worried about missing code paths like that, so I had restricted the changes to where I knew we were reading the superblock off disk. If there is a problem, it could be covered by setting c.cache_config.blksize_initialized = true within the mkfs code after the blocksize is set in f2fs_parse_options. But that isn't needed if there's no caching in mkfs.
On 2023/11/28 8:52, Daniel Rosenberg wrote: > On Sun, Nov 26, 2023 at 5:42 PM Chao Yu <chao@kernel.org> wrote: >> >> Hi Daniel, >> >> How about this? It be more explicit to indicate the logic? >> >> --- >> fsck/mount.c | 2 ++ >> include/f2fs_fs.h | 3 +++ >> lib/libf2fs_io.c | 4 ++++ >> 3 files changed, 9 insertions(+) >> >> diff --git a/fsck/mount.c b/fsck/mount.c >> index 72516f4..4dfb996 100644 >> --- a/fsck/mount.c >> +++ b/fsck/mount.c >> @@ -1238,6 +1238,8 @@ int init_sb_info(struct f2fs_sb_info *sbi) >> MSG(0, "Info: total FS sectors = %"PRIu64" (%"PRIu64" MB)\n", >> total_sectors, total_sectors >> >> (20 - get_sb(log_sectorsize))); >> + >> + c.cache_config.blksize_initialized = true; >> return 0; >> } >> >> diff --git a/include/f2fs_fs.h b/include/f2fs_fs.h >> index 6df2e73..e377d48 100644 >> --- a/include/f2fs_fs.h >> +++ b/include/f2fs_fs.h >> @@ -460,6 +460,9 @@ typedef struct { >> unsigned max_hash_collision; >> >> bool dbg_en; >> + >> + /* whether blksize has been initialized */ >> + bool blksize_initialized; >> } dev_cache_config_t; >> >> /* f2fs_configration for compression used for sload.f2fs */ >> diff --git a/lib/libf2fs_io.c b/lib/libf2fs_io.c >> index 39d3777..bb77418 100644 >> --- a/lib/libf2fs_io.c >> +++ b/lib/libf2fs_io.c >> @@ -199,6 +199,10 @@ void dcache_init(void) >> { >> long n; >> >> + /* Must not initiate cache until block size is known */ >> + if (!c.cache_config.blksize_initialized) >> + return; >> + >> if (c.cache_config.num_cache_entry <= 0) >> return; >> >> -- >> 2.40.1 >> >> > I think that works too. I was initially going to go with code like > that, but I was unsure if read/writes that aren't in the fsck path > used caching as well. There's a few uses of those in the mkfs/ paths, > but I don't think there's anything that sets > cache_config.num_cache_entry there. I was worried about missing code > paths like that, so I had restricted the changes to where I knew we > were reading the superblock off disk. If there is a problem, it could > be covered by setting c.cache_config.blksize_initialized = true within > the mkfs code after the blocksize is set in f2fs_parse_options. But > that isn't needed if there's no caching in mkfs. Oh, I think the most useful case of dcache is fsck, not sure about resize, but I don't think it will be benefit to mkfs/sload, based on IO pattern of these cases. IMO, we can get rid of dcache for mkfs/sload cases in order to avoid maintaining complicated design/logic. Thanks,
diff --git a/fsck/mount.c b/fsck/mount.c index 72516f4..658e601 100644 --- a/fsck/mount.c +++ b/fsck/mount.c @@ -3860,8 +3860,12 @@ int f2fs_do_mount(struct f2fs_sb_info *sbi) { struct f2fs_checkpoint *cp = NULL; struct f2fs_super_block *sb = NULL; + int num_cache_entry = c.cache_config.num_cache_entry; int ret; + /* Must not initiate cache until block size is known */ + c.cache_config.num_cache_entry = 0; + sbi->active_logs = NR_CURSEG_TYPE; ret = validate_super_block(sbi, SB0_ADDR); if (ret) { @@ -3881,6 +3885,7 @@ int f2fs_do_mount(struct f2fs_sb_info *sbi) } } sb = F2FS_RAW_SUPER(sbi); + c.cache_config.num_cache_entry = num_cache_entry; ret = check_sector_size(sb); if (ret)
The cache is initialized during the first read, however, it requires the block size to establish its buffer. This disables the cache until the block size is known. Signed-off-by: Daniel Rosenberg <drosen@google.com> --- fsck/mount.c | 5 +++++ 1 file changed, 5 insertions(+)