| Message ID | 39d3966f896f04c3003eb9a954ce84ff33d51345.1743731232.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | btrfs: fsstress hang fix for large data folios | expand |
On Fri, Apr 4, 2025 at 2:48 AM Qu Wenruo <wqu@suse.com> wrote: > > Inside function unlock_delalloc_folio() and lock_delalloc_folios(), we function -> functions > have the following early exist: exist -> exit > > if (index == locked_folio->index && end_index == index) > return; > > This allows us to exist early if the range are inside the same locked exist -> exit the range are inside -> the range is inside > folio. > > But even without the above early check, the existing code can handle it > well, as both __process_folios_contig() and lock_delalloc_folios() will > skip any folio page lock/unlock if it's on the locked folio. > > Just remove those unnecessary early exits. It looks good to me from a functional point of view. But without this early exits there's a bit of work done, from function calls to initializing and releasing folio batches, calling filemap_get_folios_contig() which will search the mapping's xarray and always grab one folio and add it to the batch, etc. It's not uncommon to do IO on a range not spanning more than one folio, especially when supporting large folios, which will be more likely. I understand the goal here is to remove some code, but this code is cheaper compared to all that unnecessary overhead. Have you considered that? Thanks. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/extent_io.c | 8 -------- > 1 file changed, 8 deletions(-) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index 8b29eeac3884..013268f70621 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -224,12 +224,7 @@ static noinline void unlock_delalloc_folio(const struct inode *inode, > const struct folio *locked_folio, > u64 start, u64 end) > { > - unsigned long index = start >> PAGE_SHIFT; > - unsigned long end_index = end >> PAGE_SHIFT; > - > ASSERT(locked_folio); > - if (index == locked_folio->index && end_index == index) > - return; > > __process_folios_contig(inode->i_mapping, locked_folio, start, end, > PAGE_UNLOCK); > @@ -246,9 +241,6 @@ static noinline int lock_delalloc_folios(struct inode *inode, > u64 processed_end = start; > struct folio_batch fbatch; > > - if (index == locked_folio->index && index == end_index) > - return 0; > - > folio_batch_init(&fbatch); > while (index <= end_index) { > unsigned int found_folios, i; > -- > 2.49.0 > >
在 2025/4/5 02:34, Filipe Manana 写道: > On Fri, Apr 4, 2025 at 2:48 AM Qu Wenruo <wqu@suse.com> wrote: >> >> Inside function unlock_delalloc_folio() and lock_delalloc_folios(), we > > function -> functions > >> have the following early exist: > > exist -> exit > >> >> if (index == locked_folio->index && end_index == index) >> return; >> >> This allows us to exist early if the range are inside the same locked > > exist -> exit > the range are inside -> the range is inside > >> folio. >> >> But even without the above early check, the existing code can handle it >> well, as both __process_folios_contig() and lock_delalloc_folios() will >> skip any folio page lock/unlock if it's on the locked folio. >> >> Just remove those unnecessary early exits. > > It looks good to me from a functional point of view. > > But without this early exits there's a bit of work done, from function > calls to initializing and releasing folio batches, calling > filemap_get_folios_contig() which > will search the mapping's xarray and always grab one folio and add it > to the batch, etc. > > It's not uncommon to do IO on a range not spanning more than one > folio, especially when supporting large folios, which will be more > likely. > I understand the goal here is to remove some code, but this code is > cheaper compared to all that unnecessary overhead. > > Have you considered that? Yes, but the main reason here is the usage of (folio->index == index) check. With large folios, such check is no longer reliable anyway, and initially I thought to just change it to folio_contains(), but it turns out that is not really needed either. So that's the main reason to get rid this of these early exits. Thanks, Qu > > Thanks. > >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> fs/btrfs/extent_io.c | 8 -------- >> 1 file changed, 8 deletions(-) >> >> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c >> index 8b29eeac3884..013268f70621 100644 >> --- a/fs/btrfs/extent_io.c >> +++ b/fs/btrfs/extent_io.c >> @@ -224,12 +224,7 @@ static noinline void unlock_delalloc_folio(const struct inode *inode, >> const struct folio *locked_folio, >> u64 start, u64 end) >> { >> - unsigned long index = start >> PAGE_SHIFT; >> - unsigned long end_index = end >> PAGE_SHIFT; >> - >> ASSERT(locked_folio); >> - if (index == locked_folio->index && end_index == index) >> - return; >> >> __process_folios_contig(inode->i_mapping, locked_folio, start, end, >> PAGE_UNLOCK); >> @@ -246,9 +241,6 @@ static noinline int lock_delalloc_folios(struct inode *inode, >> u64 processed_end = start; >> struct folio_batch fbatch; >> >> - if (index == locked_folio->index && index == end_index) >> - return 0; >> - >> folio_batch_init(&fbatch); >> while (index <= end_index) { >> unsigned int found_folios, i; >> -- >> 2.49.0 >> >>
On Fri, Apr 4, 2025 at 10:44 PM Qu Wenruo <wqu@suse.com> wrote: > > > > 在 2025/4/5 02:34, Filipe Manana 写道: > > On Fri, Apr 4, 2025 at 2:48 AM Qu Wenruo <wqu@suse.com> wrote: > >> > >> Inside function unlock_delalloc_folio() and lock_delalloc_folios(), we > > > > function -> functions > > > >> have the following early exist: > > > > exist -> exit > > > >> > >> if (index == locked_folio->index && end_index == index) > >> return; > >> > >> This allows us to exist early if the range are inside the same locked > > > > exist -> exit > > the range are inside -> the range is inside > > > >> folio. > >> > >> But even without the above early check, the existing code can handle it > >> well, as both __process_folios_contig() and lock_delalloc_folios() will > >> skip any folio page lock/unlock if it's on the locked folio. > >> > >> Just remove those unnecessary early exits. > > > > It looks good to me from a functional point of view. > > > > But without this early exits there's a bit of work done, from function > > calls to initializing and releasing folio batches, calling > > filemap_get_folios_contig() which > > will search the mapping's xarray and always grab one folio and add it > > to the batch, etc. > > > > It's not uncommon to do IO on a range not spanning more than one > > folio, especially when supporting large folios, which will be more > > likely. > > I understand the goal here is to remove some code, but this code is > > cheaper compared to all that unnecessary overhead. > > > > Have you considered that? > > Yes, but the main reason here is the usage of (folio->index == index) check. > > With large folios, such check is no longer reliable anyway, and > initially I thought to just change it to folio_contains(), but it turns > out that is not really needed either. > > So that's the main reason to get rid this of these early exits. Ok. Reviewed-by: Filipe Manana <fdmanana@suse.com> Thanks. > > Thanks, > Qu > > > > > Thanks. > > > >> > >> Signed-off-by: Qu Wenruo <wqu@suse.com> > >> --- > >> fs/btrfs/extent_io.c | 8 -------- > >> 1 file changed, 8 deletions(-) > >> > >> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > >> index 8b29eeac3884..013268f70621 100644 > >> --- a/fs/btrfs/extent_io.c > >> +++ b/fs/btrfs/extent_io.c > >> @@ -224,12 +224,7 @@ static noinline void unlock_delalloc_folio(const struct inode *inode, > >> const struct folio *locked_folio, > >> u64 start, u64 end) > >> { > >> - unsigned long index = start >> PAGE_SHIFT; > >> - unsigned long end_index = end >> PAGE_SHIFT; > >> - > >> ASSERT(locked_folio); > >> - if (index == locked_folio->index && end_index == index) > >> - return; > >> > >> __process_folios_contig(inode->i_mapping, locked_folio, start, end, > >> PAGE_UNLOCK); > >> @@ -246,9 +241,6 @@ static noinline int lock_delalloc_folios(struct inode *inode, > >> u64 processed_end = start; > >> struct folio_batch fbatch; > >> > >> - if (index == locked_folio->index && index == end_index) > >> - return 0; > >> - > >> folio_batch_init(&fbatch); > >> while (index <= end_index) { > >> unsigned int found_folios, i; > >> -- > >> 2.49.0 > >> > >> >
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 8b29eeac3884..013268f70621 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -224,12 +224,7 @@ static noinline void unlock_delalloc_folio(const struct inode *inode, const struct folio *locked_folio, u64 start, u64 end) { - unsigned long index = start >> PAGE_SHIFT; - unsigned long end_index = end >> PAGE_SHIFT; - ASSERT(locked_folio); - if (index == locked_folio->index && end_index == index) - return; __process_folios_contig(inode->i_mapping, locked_folio, start, end, PAGE_UNLOCK); @@ -246,9 +241,6 @@ static noinline int lock_delalloc_folios(struct inode *inode, u64 processed_end = start; struct folio_batch fbatch; - if (index == locked_folio->index && index == end_index) - return 0; - folio_batch_init(&fbatch); while (index <= end_index) { unsigned int found_folios, i;
Inside function unlock_delalloc_folio() and lock_delalloc_folios(), we have the following early exist: if (index == locked_folio->index && end_index == index) return; This allows us to exist early if the range are inside the same locked folio. But even without the above early check, the existing code can handle it well, as both __process_folios_contig() and lock_delalloc_folios() will skip any folio page lock/unlock if it's on the locked folio. Just remove those unnecessary early exits. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/extent_io.c | 8 -------- 1 file changed, 8 deletions(-)