Joseph Qi
2023-Apr-21 07:35 UTC
[Ocfs2-devel] [PATCH] ocfs2: Fix wrong search logic in __ocfs2_resv_find_window
Hi, Could you please share a reproducer? Thanks, Joseph On 4/6/23 11:40 PM, Heming Zhao wrote:> ** problem ** > > Current code triggers a defragfs bug [1]: > > ``` > tw-tst:~ # defragfs.ocfs2 /mnt/test_from_dd1 > defragfs.ocfs2 1.8.7 > [ERROR]Move extent failed:"/mnt/test_from_dd1" - No space left on device > [ERROR]"/mnt/test_from_dd1":No space left on device > ``` > > I added some debug messages in relevant functions. When above error > messages appeared, the la-window still had enough continuous bitmap > to use, the -ENOSPC is wrong. > > ** analysis ** > > __ocfs2_resv_find_window should use resv node from rb-tree to do linear > search. But current code logic uses un-managered area between two resvs. > The searching logic deviates from this func job, which should only focus > on reservation areas (when rb_root is non NULL). Another issue of > __ocfs2_resv_find_window is more inclined to generate inner fragment. > It doesn't search & consume existing reservations but be apt to create > new reservation item. > > This patch pulls this func (__ocfs2_resv_find_window) to do what it > should do: search & consume resev. if this func fails to get suitable > bitmap area, caller ocfs2_resv_find_window fallbacks to use oldest > resv from LRU to do the final search. > > [1]: https://bugzilla.suse.com/show_bug.cgi?id=1131931 > > Signed-off-by: Heming Zhao <heming.zhao at suse.com> > --- > fs/ocfs2/reservations.c | 48 +++++++++++++++++++++++++++++------------ > 1 file changed, 34 insertions(+), 14 deletions(-) > > diff --git a/fs/ocfs2/reservations.c b/fs/ocfs2/reservations.c > index a9d1296d736d..eda672622d1d 100644 > --- a/fs/ocfs2/reservations.c > +++ b/fs/ocfs2/reservations.c > @@ -458,10 +458,11 @@ static void __ocfs2_resv_find_window(struct ocfs2_reservation_map *resmap, > { > struct rb_root *root = &resmap->m_reservations; > unsigned int gap_start, gap_end, gap_len; > - struct ocfs2_alloc_reservation *prev_resv, *next_resv; > + struct ocfs2_alloc_reservation *prev_resv, *next_resv, *best_resv; > struct rb_node *prev, *next; > unsigned int cstart, clen; > unsigned int best_start = 0, best_len = 0; > + int create_new = 0; > > /* > * Nasty cases to consider: > @@ -540,8 +541,9 @@ static void __ocfs2_resv_find_window(struct ocfs2_reservation_map *resmap, > if (clen) { > best_len = clen; > best_start = cstart; > + create_new = 1; > if (best_len == wanted) > - goto out_insert; > + goto out_create; > } > > prev_resv = next_resv; > @@ -557,13 +559,9 @@ static void __ocfs2_resv_find_window(struct ocfs2_reservation_map *resmap, > while (1) { > next = rb_next(prev); > if (next) { > - next_resv = rb_entry(next, > - struct ocfs2_alloc_reservation, > - r_node); > - > - gap_start = ocfs2_resv_end(prev_resv) + 1; > - gap_end = next_resv->r_start - 1; > - gap_len = gap_end - gap_start + 1; > + gap_start = prev_resv->r_start; > + gap_end = prev_resv->r_start + prev_resv->r_len - 1; > + gap_len = prev_resv->r_len; > } else { > /* > * We're at the rightmost edge of the > @@ -575,8 +573,8 @@ static void __ocfs2_resv_find_window(struct ocfs2_reservation_map *resmap, > gap_end = resmap->m_bitmap_len - 1; > } > > - trace_ocfs2_resv_find_window_next(next ? next_resv->r_start: -1, > - next ? ocfs2_resv_end(next_resv) : -1); > + trace_ocfs2_resv_find_window_next(next ? prev_resv->r_start : -1, > + next ? ocfs2_resv_end(prev_resv) : -1); > /* > * No need to check this gap if we have already found > * a larger region of free bits. > @@ -589,10 +587,16 @@ static void __ocfs2_resv_find_window(struct ocfs2_reservation_map *resmap, > if (clen == wanted) { > best_len = clen; > best_start = cstart; > - goto out_insert; > + best_resv = prev_resv; > + if (!next) > + goto out_create; > + else > + goto out_insert; > } else if (clen > best_len) { > best_len = clen; > best_start = cstart; > + best_resv = prev_resv; > + create_new = 0; > } > > next_resv: > @@ -604,12 +608,28 @@ static void __ocfs2_resv_find_window(struct ocfs2_reservation_map *resmap, > r_node); > } > > -out_insert: > - if (best_len) { > + if (!best_len) > + return; > + > + if (create_new) { > +out_create: > resv->r_start = best_start; > resv->r_len = best_len; > ocfs2_resv_insert(resmap, resv); > + return; > } > + > +out_insert: > + if (best_resv->r_len <= wanted) { > + resv->r_start = best_start; > + resv->r_len = best_len; > + __ocfs2_resv_discard(resmap, best_resv); > + } else { > + best_resv->r_len -= best_len; > + resv->r_start = ocfs2_resv_end(best_resv) + 1; > + resv->r_len = best_len; > + } > + ocfs2_resv_insert(resmap, resv); > } > > static void ocfs2_cannibalize_resv(struct ocfs2_reservation_map *resmap,
Heming Zhao
2023-Apr-21 08:01 UTC
[Ocfs2-devel] [PATCH] ocfs2: Fix wrong search logic in __ocfs2_resv_find_window
On Fri, Apr 21, 2023 at 03:35:01PM +0800, Joseph Qi wrote:> Hi, > Could you please share a reproducer? >Anyone could access & download the URL [1] (I wrote it in patch commit log) without register SUSE account. Please check attachment file, which I downloaded from [1] and modified under the BZ comment 1. The trigger method is also in comment 1, I copy here: ./defragfs_test.sh -d /dev/vdf -m /mnt -n ocfstst -s pcmk -o /tmp -b 1024 -c 8192 note: you may modify '-n' (cluster name) & -s' (cluster stack). [1]: https://bugzilla.suse.com/show_bug.cgi?id=1131931 Thanks, Heming> > On 4/6/23 11:40 PM, Heming Zhao wrote: > > ** problem ** > > > > Current code triggers a defragfs bug [1]: > > > > ``` > > tw-tst:~ # defragfs.ocfs2 /mnt/test_from_dd1 > > defragfs.ocfs2 1.8.7 > > [ERROR]Move extent failed:"/mnt/test_from_dd1" - No space left on device > > [ERROR]"/mnt/test_from_dd1":No space left on device > > ``` > > > > I added some debug messages in relevant functions. When above error > > messages appeared, the la-window still had enough continuous bitmap > > to use, the -ENOSPC is wrong. > > > > ** analysis ** > > > > __ocfs2_resv_find_window should use resv node from rb-tree to do linear > > search. But current code logic uses un-managered area between two resvs. > > The searching logic deviates from this func job, which should only focus > > on reservation areas (when rb_root is non NULL). Another issue of > > __ocfs2_resv_find_window is more inclined to generate inner fragment. > > It doesn't search & consume existing reservations but be apt to create > > new reservation item. > > > > This patch pulls this func (__ocfs2_resv_find_window) to do what it > > should do: search & consume resev. if this func fails to get suitable > > bitmap area, caller ocfs2_resv_find_window fallbacks to use oldest > > resv from LRU to do the final search. > > > > [1]: https://bugzilla.suse.com/show_bug.cgi?id=1131931 > > > > Signed-off-by: Heming Zhao <heming.zhao at suse.com> > > --- > > fs/ocfs2/reservations.c | 48 +++++++++++++++++++++++++++++------------ > > 1 file changed, 34 insertions(+), 14 deletions(-)
Heming Zhao
2023-Apr-29 06:37 UTC
[Ocfs2-devel] [PATCH] ocfs2: Fix wrong search logic in __ocfs2_resv_find_window
ping... On 4/21/23 3:35 PM, Joseph Qi wrote:> Hi, > Could you please share a reproducer? > > Thanks, > Joseph > > On 4/6/23 11:40 PM, Heming Zhao wrote: >> ** problem ** >> >> Current code triggers a defragfs bug [1]: >> >> ``` >> tw-tst:~ # defragfs.ocfs2 /mnt/test_from_dd1 >> defragfs.ocfs2 1.8.7 >> [ERROR]Move extent failed:"/mnt/test_from_dd1" - No space left on device >> [ERROR]"/mnt/test_from_dd1":No space left on device >> ``` >> >> I added some debug messages in relevant functions. When above error >> messages appeared, the la-window still had enough continuous bitmap >> to use, the -ENOSPC is wrong. >> >> ** analysis ** >> >> __ocfs2_resv_find_window should use resv node from rb-tree to do linear >> search. But current code logic uses un-managered area between two resvs. >> The searching logic deviates from this func job, which should only focus >> on reservation areas (when rb_root is non NULL). Another issue of >> __ocfs2_resv_find_window is more inclined to generate inner fragment. >> It doesn't search & consume existing reservations but be apt to create >> new reservation item. >> >> This patch pulls this func (__ocfs2_resv_find_window) to do what it >> should do: search & consume resev. if this func fails to get suitable >> bitmap area, caller ocfs2_resv_find_window fallbacks to use oldest >> resv from LRU to do the final search. >> >> [1]: https://bugzilla.suse.com/show_bug.cgi?id=1131931 >> >> Signed-off-by: Heming Zhao <heming.zhao at suse.com> >> --- >> fs/ocfs2/reservations.c | 48 +++++++++++++++++++++++++++++------------ >> 1 file changed, 34 insertions(+), 14 deletions(-) >> >> diff --git a/fs/ocfs2/reservations.c b/fs/ocfs2/reservations.c >> index a9d1296d736d..eda672622d1d 100644 >> --- a/fs/ocfs2/reservations.c >> +++ b/fs/ocfs2/reservations.c >> @@ -458,10 +458,11 @@ static void __ocfs2_resv_find_window(struct ocfs2_reservation_map *resmap, >> { >> struct rb_root *root = &resmap->m_reservations; >> unsigned int gap_start, gap_end, gap_len; >> - struct ocfs2_alloc_reservation *prev_resv, *next_resv; >> + struct ocfs2_alloc_reservation *prev_resv, *next_resv, *best_resv; >> struct rb_node *prev, *next; >> unsigned int cstart, clen; >> unsigned int best_start = 0, best_len = 0; >> + int create_new = 0; >> >> /* >> * Nasty cases to consider: >> @@ -540,8 +541,9 @@ static void __ocfs2_resv_find_window(struct ocfs2_reservation_map *resmap, >> if (clen) { >> best_len = clen; >> best_start = cstart; >> + create_new = 1; >> if (best_len == wanted) >> - goto out_insert; >> + goto out_create; >> } >> >> prev_resv = next_resv; >> @@ -557,13 +559,9 @@ static void __ocfs2_resv_find_window(struct ocfs2_reservation_map *resmap, >> while (1) { >> next = rb_next(prev); >> if (next) { >> - next_resv = rb_entry(next, >> - struct ocfs2_alloc_reservation, >> - r_node); >> - >> - gap_start = ocfs2_resv_end(prev_resv) + 1; >> - gap_end = next_resv->r_start - 1; >> - gap_len = gap_end - gap_start + 1; >> + gap_start = prev_resv->r_start; >> + gap_end = prev_resv->r_start + prev_resv->r_len - 1; >> + gap_len = prev_resv->r_len; >> } else { >> /* >> * We're at the rightmost edge of the >> @@ -575,8 +573,8 @@ static void __ocfs2_resv_find_window(struct ocfs2_reservation_map *resmap, >> gap_end = resmap->m_bitmap_len - 1; >> } >> >> - trace_ocfs2_resv_find_window_next(next ? next_resv->r_start: -1, >> - next ? ocfs2_resv_end(next_resv) : -1); >> + trace_ocfs2_resv_find_window_next(next ? prev_resv->r_start : -1, >> + next ? ocfs2_resv_end(prev_resv) : -1); >> /* >> * No need to check this gap if we have already found >> * a larger region of free bits. >> @@ -589,10 +587,16 @@ static void __ocfs2_resv_find_window(struct ocfs2_reservation_map *resmap, >> if (clen == wanted) { >> best_len = clen; >> best_start = cstart; >> - goto out_insert; >> + best_resv = prev_resv; >> + if (!next) >> + goto out_create; >> + else >> + goto out_insert; >> } else if (clen > best_len) { >> best_len = clen; >> best_start = cstart; >> + best_resv = prev_resv; >> + create_new = 0; >> } >> >> next_resv: >> @@ -604,12 +608,28 @@ static void __ocfs2_resv_find_window(struct ocfs2_reservation_map *resmap, >> r_node); >> } >> >> -out_insert: >> - if (best_len) { >> + if (!best_len) >> + return; >> + >> + if (create_new) { >> +out_create: >> resv->r_start = best_start; >> resv->r_len = best_len; >> ocfs2_resv_insert(resmap, resv); >> + return; >> } >> + >> +out_insert: >> + if (best_resv->r_len <= wanted) { >> + resv->r_start = best_start; >> + resv->r_len = best_len; >> + __ocfs2_resv_discard(resmap, best_resv); >> + } else { >> + best_resv->r_len -= best_len; >> + resv->r_start = ocfs2_resv_end(best_resv) + 1; >> + resv->r_len = best_len; >> + } >> + ocfs2_resv_insert(resmap, resv); >> } >> >> static void ocfs2_cannibalize_resv(struct ocfs2_reservation_map *resmap,