| Message ID | 20230217012000.5774-1-heming.zhao@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ocfs2: fix non-auto defrag path not working issue | expand |
On 2/17/23 9:20 AM, Heming Zhao wrote: > This commit fixes three issues on non-auto defrag path (defragfs.ocfs2 > doesn't set OCFS2_MOVE_EXT_FL_AUTO_DEFRAG on range.me_flags): > - For ocfs2_find_victim_alloc_group(), old code forgot enlarge bitmap > range for global_bitmap case. Old code could generate negative > vict_bit. > - For ocfs2_probe_alloc_group(), old code forgot back off move_len when > finding enough bitmap space. Old code has possibility to make data > corruption. > - For ocfs2_ioctl_move_extents(), this func should set me_threshold for > both auto & non-auto path. Without setting me_threshold, > ocfs2_move_extent() will make 'move_max_hop = 0', then > ocfs2_probe_alloc_group() returns 'phys_cpos == 0' under max_hop is 0. > > Another info is current defragfs.ocfs2 doesn't have ability to trigger > non-auto defrag path. I modified defragfs.ocfs2 source code ".me_flags" > and hardcode ".me_goal" for running test. > I'm fine with the code changes. But the commit log needs improve. How about the following? This fixes three issues on move extents ioctl without auto defrag: a) In ocfs2_find_victim_alloc_group(), we have to convert bits to block first in case of global bitmap. b) In ocfs2_probe_alloc_group(), when finding enough bits in block group bitmap, we have to back off move_len to start pos as well, otherwise it may corrupt filesystem. c) In ocfs2_ioctl_move_extents(), set me_threshold both for non-auto and auto defrag paths. Otherwise it will set move_max_hop to 0 and finally cause unexpectedly ENOSPC error. Currently there is no tools triggering the above issues since defragfs.ocfs2 enables auto defrag by default. Test with manually changing defragfs.ocfs2 to run non auto defrag path. Thanks, Joseph > Signed-off-by: Heming Zhao <heming.zhao@suse.com> > --- > fs/ocfs2/move_extents.c | 24 +++++++++++++----------- > 1 file changed, 13 insertions(+), 11 deletions(-) > > diff --git a/fs/ocfs2/move_extents.c b/fs/ocfs2/move_extents.c > index 6251748c695b..b1e32ec4a9d4 100644 > --- a/fs/ocfs2/move_extents.c > +++ b/fs/ocfs2/move_extents.c > @@ -434,7 +434,7 @@ static int ocfs2_find_victim_alloc_group(struct inode *inode, > bg = (struct ocfs2_group_desc *)gd_bh->b_data; > > if (vict_blkno < (le64_to_cpu(bg->bg_blkno) + > - le16_to_cpu(bg->bg_bits))) { > + (le16_to_cpu(bg->bg_bits) << bits_per_unit))) { > > *ret_bh = gd_bh; > *vict_bit = (vict_blkno - blkno) >> > @@ -549,6 +549,7 @@ static void ocfs2_probe_alloc_group(struct inode *inode, struct buffer_head *bh, > last_free_bits++; > > if (last_free_bits == move_len) { > + i -= move_len; > *goal_bit = i; > *phys_cpos = base_cpos + i; > break; > @@ -1020,18 +1021,19 @@ int ocfs2_ioctl_move_extents(struct file *filp, void __user *argp) > > context->range = ⦥ > > + /* > + * ok, the default theshold for the defragmentation > + * is 1M, since our maximum clustersize was 1M also. > + * any thought? > + */ > + if (!range.me_threshold) > + range.me_threshold = 1024 * 1024; > + > + if (range.me_threshold > i_size_read(inode)) > + range.me_threshold = i_size_read(inode); > + > if (range.me_flags & OCFS2_MOVE_EXT_FL_AUTO_DEFRAG) { > context->auto_defrag = 1; > - /* > - * ok, the default theshold for the defragmentation > - * is 1M, since our maximum clustersize was 1M also. > - * any thought? > - */ > - if (!range.me_threshold) > - range.me_threshold = 1024 * 1024; > - > - if (range.me_threshold > i_size_read(inode)) > - range.me_threshold = i_size_read(inode); > > if (range.me_flags & OCFS2_MOVE_EXT_FL_PART_DEFRAG) > context->partial = 1;
On 2/20/23 11:18 AM, Joseph Qi wrote: > > > On 2/17/23 9:20 AM, Heming Zhao wrote: >> This commit fixes three issues on non-auto defrag path (defragfs.ocfs2 >> doesn't set OCFS2_MOVE_EXT_FL_AUTO_DEFRAG on range.me_flags): >> - For ocfs2_find_victim_alloc_group(), old code forgot enlarge bitmap >> range for global_bitmap case. Old code could generate negative >> vict_bit. >> - For ocfs2_probe_alloc_group(), old code forgot back off move_len when >> finding enough bitmap space. Old code has possibility to make data >> corruption. >> - For ocfs2_ioctl_move_extents(), this func should set me_threshold for >> both auto & non-auto path. Without setting me_threshold, >> ocfs2_move_extent() will make 'move_max_hop = 0', then >> ocfs2_probe_alloc_group() returns 'phys_cpos == 0' under max_hop is 0. >> >> Another info is current defragfs.ocfs2 doesn't have ability to trigger >> non-auto defrag path. I modified defragfs.ocfs2 source code ".me_flags" >> and hardcode ".me_goal" for running test. >> > > I'm fine with the code changes. But the commit log needs improve. > How about the following? OK, I will send v2 patch. Thank you for your review and spent time to revise my commit log. Thanks, Heming > > This fixes three issues on move extents ioctl without auto defrag: > > a) In ocfs2_find_victim_alloc_group(), we have to convert bits to block > first in case of global bitmap. > b) In ocfs2_probe_alloc_group(), when finding enough bits in block group > bitmap, we have to back off move_len to start pos as well, otherwise it > may corrupt filesystem. > c) In ocfs2_ioctl_move_extents(), set me_threshold both for non-auto and > auto defrag paths. Otherwise it will set move_max_hop to 0 and finally > cause unexpectedly ENOSPC error. > > Currently there is no tools triggering the above issues since > defragfs.ocfs2 enables auto defrag by default. Test with manually > changing defragfs.ocfs2 to run non auto defrag path. > > Thanks, > Joseph > > >> Signed-off-by: Heming Zhao <heming.zhao@suse.com> >> --- >> fs/ocfs2/move_extents.c | 24 +++++++++++++----------- >> 1 file changed, 13 insertions(+), 11 deletions(-) >>
diff --git a/fs/ocfs2/move_extents.c b/fs/ocfs2/move_extents.c index 6251748c695b..b1e32ec4a9d4 100644 --- a/fs/ocfs2/move_extents.c +++ b/fs/ocfs2/move_extents.c @@ -434,7 +434,7 @@ static int ocfs2_find_victim_alloc_group(struct inode *inode, bg = (struct ocfs2_group_desc *)gd_bh->b_data; if (vict_blkno < (le64_to_cpu(bg->bg_blkno) + - le16_to_cpu(bg->bg_bits))) { + (le16_to_cpu(bg->bg_bits) << bits_per_unit))) { *ret_bh = gd_bh; *vict_bit = (vict_blkno - blkno) >> @@ -549,6 +549,7 @@ static void ocfs2_probe_alloc_group(struct inode *inode, struct buffer_head *bh, last_free_bits++; if (last_free_bits == move_len) { + i -= move_len; *goal_bit = i; *phys_cpos = base_cpos + i; break; @@ -1020,18 +1021,19 @@ int ocfs2_ioctl_move_extents(struct file *filp, void __user *argp) context->range = ⦥ + /* + * ok, the default theshold for the defragmentation + * is 1M, since our maximum clustersize was 1M also. + * any thought? + */ + if (!range.me_threshold) + range.me_threshold = 1024 * 1024; + + if (range.me_threshold > i_size_read(inode)) + range.me_threshold = i_size_read(inode); + if (range.me_flags & OCFS2_MOVE_EXT_FL_AUTO_DEFRAG) { context->auto_defrag = 1; - /* - * ok, the default theshold for the defragmentation - * is 1M, since our maximum clustersize was 1M also. - * any thought? - */ - if (!range.me_threshold) - range.me_threshold = 1024 * 1024; - - if (range.me_threshold > i_size_read(inode)) - range.me_threshold = i_size_read(inode); if (range.me_flags & OCFS2_MOVE_EXT_FL_PART_DEFRAG) context->partial = 1;
This commit fixes three issues on non-auto defrag path (defragfs.ocfs2 doesn't set OCFS2_MOVE_EXT_FL_AUTO_DEFRAG on range.me_flags): - For ocfs2_find_victim_alloc_group(), old code forgot enlarge bitmap range for global_bitmap case. Old code could generate negative vict_bit. - For ocfs2_probe_alloc_group(), old code forgot back off move_len when finding enough bitmap space. Old code has possibility to make data corruption. - For ocfs2_ioctl_move_extents(), this func should set me_threshold for both auto & non-auto path. Without setting me_threshold, ocfs2_move_extent() will make 'move_max_hop = 0', then ocfs2_probe_alloc_group() returns 'phys_cpos == 0' under max_hop is 0. Another info is current defragfs.ocfs2 doesn't have ability to trigger non-auto defrag path. I modified defragfs.ocfs2 source code ".me_flags" and hardcode ".me_goal" for running test. Signed-off-by: Heming Zhao <heming.zhao@suse.com> --- fs/ocfs2/move_extents.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-)