| Message ID | 20240226013208.2389246-1-chao@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8249aac1b05c28f3b35363b844b5b0194f838052 |
| Headers | show |
| Series | [f2fs-dev,1/4] f2fs: fix blkofs_end correctly in f2fs_migrate_blocks() | expand |
Jaegeuk, Daeho, Any comments on this serials? Thanks, On 2024/2/26 9:32, Chao Yu wrote: > In f2fs_migrate_blocks(), when traversing blocks in last section, > blkofs_end should be (start_blk + blkcnt - 1) % blk_per_sec, fix it. > > Signed-off-by: Chao Yu <chao@kernel.org> > --- > fs/f2fs/data.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > index c21b92f18463..0c728e82d936 100644 > --- a/fs/f2fs/data.c > +++ b/fs/f2fs/data.c > @@ -3841,13 +3841,14 @@ static int f2fs_migrate_blocks(struct inode *inode, block_t start_blk, > struct f2fs_sb_info *sbi = F2FS_I_SB(inode); > unsigned int blkofs; > unsigned int blk_per_sec = BLKS_PER_SEC(sbi); > + unsigned int end_blk = start_blk + blkcnt - 1; > unsigned int secidx = start_blk / blk_per_sec; > unsigned int end_sec; > int ret = 0; > > if (!blkcnt) > return 0; > - end_sec = secidx + (blkcnt - 1) / blk_per_sec; > + end_sec = end_blk / blk_per_sec; > > f2fs_down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]); > filemap_invalidate_lock(inode->i_mapping); > @@ -3857,7 +3858,7 @@ static int f2fs_migrate_blocks(struct inode *inode, block_t start_blk, > > for (; secidx <= end_sec; secidx++) { > unsigned int blkofs_end = secidx == end_sec ? > - (blkcnt - 1) % blk_per_sec : blk_per_sec - 1; > + end_blk % blk_per_sec : blk_per_sec - 1; > > f2fs_down_write(&sbi->pin_sem); >
On Thu, Feb 29, 2024 at 2:11 AM Chao Yu <chao@kernel.org> wrote: > > Jaegeuk, Daeho, > > Any comments on this serials? > > Thanks, No functional difference here, since start_blk is always aligned with the section address. However, this is more clear in itself. Reviewed-by: Daeho Jeong <daehojeong@google.com> Thanks, > > On 2024/2/26 9:32, Chao Yu wrote: > > In f2fs_migrate_blocks(), when traversing blocks in last section, > > blkofs_end should be (start_blk + blkcnt - 1) % blk_per_sec, fix it. > > > > Signed-off-by: Chao Yu <chao@kernel.org> > > --- > > fs/f2fs/data.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > > index c21b92f18463..0c728e82d936 100644 > > --- a/fs/f2fs/data.c > > +++ b/fs/f2fs/data.c > > @@ -3841,13 +3841,14 @@ static int f2fs_migrate_blocks(struct inode *inode, block_t start_blk, > > struct f2fs_sb_info *sbi = F2FS_I_SB(inode); > > unsigned int blkofs; > > unsigned int blk_per_sec = BLKS_PER_SEC(sbi); > > + unsigned int end_blk = start_blk + blkcnt - 1; > > unsigned int secidx = start_blk / blk_per_sec; > > unsigned int end_sec; > > int ret = 0; > > > > if (!blkcnt) > > return 0; > > - end_sec = secidx + (blkcnt - 1) / blk_per_sec; > > + end_sec = end_blk / blk_per_sec; > > > > f2fs_down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]); > > filemap_invalidate_lock(inode->i_mapping); > > @@ -3857,7 +3858,7 @@ static int f2fs_migrate_blocks(struct inode *inode, block_t start_blk, > > > > for (; secidx <= end_sec; secidx++) { > > unsigned int blkofs_end = secidx == end_sec ? > > - (blkcnt - 1) % blk_per_sec : blk_per_sec - 1; > > + end_blk % blk_per_sec : blk_per_sec - 1; > > > > f2fs_down_write(&sbi->pin_sem); > > > > > _______________________________________________ > Linux-f2fs-devel mailing list > Linux-f2fs-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
On 2024/3/1 1:41, Daeho Jeong wrote: > On Thu, Feb 29, 2024 at 2:11 AM Chao Yu <chao@kernel.org> wrote: >> >> Jaegeuk, Daeho, >> >> Any comments on this serials? >> >> Thanks, > > No functional difference here, since start_blk is always aligned with > the section address. You're right. > However, this is more clear in itself. Thanks for the review! One more thing is, I found that fallocate() on pinned file will preallocate aligned w/ section-size which is about several hundred megabyte for ZUFS case, since commit e1175f022911 ("f2fs: fix to align to section for fallocate() on pinned file"). It looks not make sense, especially for logcat case which actually want to preallocate 2MB space, so, what about reverting commit e1175f022911 and looking for other solution to avoid GCing on fragmented pinned file. What do you think? Thanks, > > Reviewed-by: Daeho Jeong <daehojeong@google.com> > > Thanks, > >> >> On 2024/2/26 9:32, Chao Yu wrote: >>> In f2fs_migrate_blocks(), when traversing blocks in last section, >>> blkofs_end should be (start_blk + blkcnt - 1) % blk_per_sec, fix it. >>> >>> Signed-off-by: Chao Yu <chao@kernel.org> >>> --- >>> fs/f2fs/data.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c >>> index c21b92f18463..0c728e82d936 100644 >>> --- a/fs/f2fs/data.c >>> +++ b/fs/f2fs/data.c >>> @@ -3841,13 +3841,14 @@ static int f2fs_migrate_blocks(struct inode *inode, block_t start_blk, >>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode); >>> unsigned int blkofs; >>> unsigned int blk_per_sec = BLKS_PER_SEC(sbi); >>> + unsigned int end_blk = start_blk + blkcnt - 1; >>> unsigned int secidx = start_blk / blk_per_sec; >>> unsigned int end_sec; >>> int ret = 0; >>> >>> if (!blkcnt) >>> return 0; >>> - end_sec = secidx + (blkcnt - 1) / blk_per_sec; >>> + end_sec = end_blk / blk_per_sec; >>> >>> f2fs_down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]); >>> filemap_invalidate_lock(inode->i_mapping); >>> @@ -3857,7 +3858,7 @@ static int f2fs_migrate_blocks(struct inode *inode, block_t start_blk, >>> >>> for (; secidx <= end_sec; secidx++) { >>> unsigned int blkofs_end = secidx == end_sec ? >>> - (blkcnt - 1) % blk_per_sec : blk_per_sec - 1; >>> + end_blk % blk_per_sec : blk_per_sec - 1; >>> >>> f2fs_down_write(&sbi->pin_sem); >>> >> >> >> _______________________________________________ >> Linux-f2fs-devel mailing list >> Linux-f2fs-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
On 03/01, Chao Yu wrote: > On 2024/3/1 1:41, Daeho Jeong wrote: > > On Thu, Feb 29, 2024 at 2:11 AM Chao Yu <chao@kernel.org> wrote: > > > > > > Jaegeuk, Daeho, > > > > > > Any comments on this serials? > > > > > > Thanks, > > > > No functional difference here, since start_blk is always aligned with > > the section address. > > You're right. > > > However, this is more clear in itself. > > Thanks for the review! > > One more thing is, I found that fallocate() on pinned file will preallocate > aligned w/ section-size which is about several hundred megabyte for ZUFS case, > since commit e1175f022911 ("f2fs: fix to align to section for fallocate() on > pinned file"). > > It looks not make sense, especially for logcat case which actually want to > preallocate 2MB space, so, what about reverting commit e1175f022911 and > looking for other solution to avoid GCing on fragmented pinned file. I remember we removed the logcat case. > > What do you think? > > Thanks, > > > > > Reviewed-by: Daeho Jeong <daehojeong@google.com> > > > > Thanks, > > > > > > > > On 2024/2/26 9:32, Chao Yu wrote: > > > > In f2fs_migrate_blocks(), when traversing blocks in last section, > > > > blkofs_end should be (start_blk + blkcnt - 1) % blk_per_sec, fix it. > > > > > > > > Signed-off-by: Chao Yu <chao@kernel.org> > > > > --- > > > > fs/f2fs/data.c | 5 +++-- > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > > > > index c21b92f18463..0c728e82d936 100644 > > > > --- a/fs/f2fs/data.c > > > > +++ b/fs/f2fs/data.c > > > > @@ -3841,13 +3841,14 @@ static int f2fs_migrate_blocks(struct inode *inode, block_t start_blk, > > > > struct f2fs_sb_info *sbi = F2FS_I_SB(inode); > > > > unsigned int blkofs; > > > > unsigned int blk_per_sec = BLKS_PER_SEC(sbi); > > > > + unsigned int end_blk = start_blk + blkcnt - 1; > > > > unsigned int secidx = start_blk / blk_per_sec; > > > > unsigned int end_sec; > > > > int ret = 0; > > > > > > > > if (!blkcnt) > > > > return 0; > > > > - end_sec = secidx + (blkcnt - 1) / blk_per_sec; > > > > + end_sec = end_blk / blk_per_sec; > > > > > > > > f2fs_down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]); > > > > filemap_invalidate_lock(inode->i_mapping); > > > > @@ -3857,7 +3858,7 @@ static int f2fs_migrate_blocks(struct inode *inode, block_t start_blk, > > > > > > > > for (; secidx <= end_sec; secidx++) { > > > > unsigned int blkofs_end = secidx == end_sec ? > > > > - (blkcnt - 1) % blk_per_sec : blk_per_sec - 1; > > > > + end_blk % blk_per_sec : blk_per_sec - 1; > > > > > > > > f2fs_down_write(&sbi->pin_sem); > > > > > > > > > > > > > _______________________________________________ > > > Linux-f2fs-devel mailing list > > > Linux-f2fs-devel@lists.sourceforge.net > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Hello: This series was applied to jaegeuk/f2fs.git (dev) by Jaegeuk Kim <jaegeuk@kernel.org>: On Mon, 26 Feb 2024 09:32:05 +0800 you wrote: > In f2fs_migrate_blocks(), when traversing blocks in last section, > blkofs_end should be (start_blk + blkcnt - 1) % blk_per_sec, fix it. > > Signed-off-by: Chao Yu <chao@kernel.org> > --- > fs/f2fs/data.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) Here is the summary with links: - [f2fs-dev,1/4] f2fs: fix blkofs_end correctly in f2fs_migrate_blocks() https://git.kernel.org/jaegeuk/f2fs/c/8249aac1b05c - [f2fs-dev,2/4] f2fs: relocate f2fs_precache_extents() in f2fs_swap_activate() https://git.kernel.org/jaegeuk/f2fs/c/f1e7646a8cd4 - [f2fs-dev,3/4] f2fs: clean up new_curseg() https://git.kernel.org/jaegeuk/f2fs/c/1081b5121b27 - [f2fs-dev,4/4] f2fs: fix to reset fields for unloaded curseg https://git.kernel.org/jaegeuk/f2fs/c/42a80aacb76b You are awesome, thank you!
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index c21b92f18463..0c728e82d936 100644 --- a/fs/f2fs/data.c +++ b/fs/f2fs/data.c @@ -3841,13 +3841,14 @@ static int f2fs_migrate_blocks(struct inode *inode, block_t start_blk, struct f2fs_sb_info *sbi = F2FS_I_SB(inode); unsigned int blkofs; unsigned int blk_per_sec = BLKS_PER_SEC(sbi); + unsigned int end_blk = start_blk + blkcnt - 1; unsigned int secidx = start_blk / blk_per_sec; unsigned int end_sec; int ret = 0; if (!blkcnt) return 0; - end_sec = secidx + (blkcnt - 1) / blk_per_sec; + end_sec = end_blk / blk_per_sec; f2fs_down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]); filemap_invalidate_lock(inode->i_mapping); @@ -3857,7 +3858,7 @@ static int f2fs_migrate_blocks(struct inode *inode, block_t start_blk, for (; secidx <= end_sec; secidx++) { unsigned int blkofs_end = secidx == end_sec ? - (blkcnt - 1) % blk_per_sec : blk_per_sec - 1; + end_blk % blk_per_sec : blk_per_sec - 1; f2fs_down_write(&sbi->pin_sem);
In f2fs_migrate_blocks(), when traversing blocks in last section, blkofs_end should be (start_blk + blkcnt - 1) % blk_per_sec, fix it. Signed-off-by: Chao Yu <chao@kernel.org> --- fs/f2fs/data.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)