| Message ID | cd8e40a516d86d1c58a221fa8d964a04bc226891.1704924693.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: don't unconditionally call folio_start_writeback in subpage | expand |
On 1/11/24 06:14, Josef Bacik wrote: > In the normal case we check if a page is under writeback and skip it > before we attempt to begin writeback. > > The exception is subpage metadata writes, where we know we don't have an > eb under writeback and we're doing it one eb at a time. Since > b5612c368648 ("mm: return void from folio_start_writeback() and related > functions") we now will BUG_ON() if we call folio_start_writeback() > on a folio that's already under writeback. Previously > folio_start_writeback() would bail if writeback was already started. > > Fix this in the subpage code by checking if we have writeback set and > skipping it if we do. This fixes the panic we were seeing on subpage. The panic stack trace in the git commit log will add more clarity. Can we fold this into the commit 55151ea9ec1b ("btrfs: migrate subpage code to folio interfaces") > Signed-off-by: Josef Bacik <josef@toxicpanda.com> If not: Reviewed-by: Anand Jain <anand.jain@oracle.com> Thanks, Anand > --- > fs/btrfs/subpage.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/subpage.c b/fs/btrfs/subpage.c > index 93511d54abf8..0e49dab8dad2 100644 > --- a/fs/btrfs/subpage.c > +++ b/fs/btrfs/subpage.c > @@ -475,7 +475,8 @@ void btrfs_subpage_set_writeback(const struct btrfs_fs_info *fs_info, > > spin_lock_irqsave(&subpage->lock, flags); > bitmap_set(subpage->bitmaps, start_bit, len >> fs_info->sectorsize_bits); > - folio_start_writeback(folio); > + if (!folio_test_writeback(folio)) > + folio_start_writeback(folio); > spin_unlock_irqrestore(&subpage->lock, flags); > } >
On 2024/1/11 08:44, Josef Bacik wrote: > In the normal case we check if a page is under writeback and skip it > before we attempt to begin writeback. > > The exception is subpage metadata writes, where we know we don't have an > eb under writeback and we're doing it one eb at a time. Since > b5612c368648 ("mm: return void from folio_start_writeback() and related > functions") we now will BUG_ON() if we call folio_start_writeback() > on a folio that's already under writeback. Previously > folio_start_writeback() would bail if writeback was already started. > > Fix this in the subpage code by checking if we have writeback set and > skipping it if we do. This fixes the panic we were seeing on subpage. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Initially I thought my subpage code screwed up again, but turns out it's MM layer behavior change. Thanks, Qu > --- > fs/btrfs/subpage.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/subpage.c b/fs/btrfs/subpage.c > index 93511d54abf8..0e49dab8dad2 100644 > --- a/fs/btrfs/subpage.c > +++ b/fs/btrfs/subpage.c > @@ -475,7 +475,8 @@ void btrfs_subpage_set_writeback(const struct btrfs_fs_info *fs_info, > > spin_lock_irqsave(&subpage->lock, flags); > bitmap_set(subpage->bitmaps, start_bit, len >> fs_info->sectorsize_bits); > - folio_start_writeback(folio); > + if (!folio_test_writeback(folio)) > + folio_start_writeback(folio); > spin_unlock_irqrestore(&subpage->lock, flags); > } >
On 2024/1/11 16:27, Anand Jain wrote: > On 1/11/24 06:14, Josef Bacik wrote: >> In the normal case we check if a page is under writeback and skip it >> before we attempt to begin writeback. >> >> The exception is subpage metadata writes, where we know we don't have an >> eb under writeback and we're doing it one eb at a time. Since >> b5612c368648 ("mm: return void from folio_start_writeback() and related >> functions") we now will BUG_ON() if we call folio_start_writeback() >> on a folio that's already under writeback. Previously >> folio_start_writeback() would bail if writeback was already started. >> > >> Fix this in the subpage code by checking if we have writeback set and >> skipping it if we do. This fixes the panic we were seeing on subpage. > > The panic stack trace in the git commit log will add more clarity. > > Can we fold this into the commit 55151ea9ec1b ("btrfs: migrate subpage > code to folio interfaces") I don't think it's a good idea to merge a bug fix to a pure refactor. This is really some conflicts between us and mm layer. Thanks, Qu > >> Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > If not: > Reviewed-by: Anand Jain <anand.jain@oracle.com> > > Thanks, Anand > > >> --- >> fs/btrfs/subpage.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/fs/btrfs/subpage.c b/fs/btrfs/subpage.c >> index 93511d54abf8..0e49dab8dad2 100644 >> --- a/fs/btrfs/subpage.c >> +++ b/fs/btrfs/subpage.c >> @@ -475,7 +475,8 @@ void btrfs_subpage_set_writeback(const struct >> btrfs_fs_info *fs_info, >> spin_lock_irqsave(&subpage->lock, flags); >> bitmap_set(subpage->bitmaps, start_bit, len >> >> fs_info->sectorsize_bits); >> - folio_start_writeback(folio); >> + if (!folio_test_writeback(folio)) >> + folio_start_writeback(folio); >> spin_unlock_irqrestore(&subpage->lock, flags); >> } > >
On Thu, Jan 11, 2024 at 01:57:13PM +0800, Anand Jain wrote: > On 1/11/24 06:14, Josef Bacik wrote: > > In the normal case we check if a page is under writeback and skip it > > before we attempt to begin writeback. > > > > The exception is subpage metadata writes, where we know we don't have an > > eb under writeback and we're doing it one eb at a time. Since > > b5612c368648 ("mm: return void from folio_start_writeback() and related > > functions") we now will BUG_ON() if we call folio_start_writeback() > > on a folio that's already under writeback. Previously > > folio_start_writeback() would bail if writeback was already started. > > > > > Fix this in the subpage code by checking if we have writeback set and > > skipping it if we do. This fixes the panic we were seeing on subpage. > > The panic stack trace in the git commit log will add more clarity. > > Can we fold this into the commit 55151ea9ec1b ("btrfs: migrate subpage > code to folio interfaces") No we can't fold that it's been already merged to master branch, besides the fact that the whole patch queue was frozen a month ago.
On Wed, Jan 10, 2024 at 05:14:21PM -0500, Josef Bacik wrote: > In the normal case we check if a page is under writeback and skip it > before we attempt to begin writeback. > > The exception is subpage metadata writes, where we know we don't have an > eb under writeback and we're doing it one eb at a time. Since > b5612c368648 ("mm: return void from folio_start_writeback() and related > functions") we now will BUG_ON() if we call folio_start_writeback() > on a folio that's already under writeback. Previously > folio_start_writeback() would bail if writeback was already started. > > Fix this in the subpage code by checking if we have writeback set and > skipping it if we do. This fixes the panic we were seeing on subpage. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> Reviewed-by: David Sterba <dsterba@suse.com>
diff --git a/fs/btrfs/subpage.c b/fs/btrfs/subpage.c index 93511d54abf8..0e49dab8dad2 100644 --- a/fs/btrfs/subpage.c +++ b/fs/btrfs/subpage.c @@ -475,7 +475,8 @@ void btrfs_subpage_set_writeback(const struct btrfs_fs_info *fs_info, spin_lock_irqsave(&subpage->lock, flags); bitmap_set(subpage->bitmaps, start_bit, len >> fs_info->sectorsize_bits); - folio_start_writeback(folio); + if (!folio_test_writeback(folio)) + folio_start_writeback(folio); spin_unlock_irqrestore(&subpage->lock, flags); }
In the normal case we check if a page is under writeback and skip it before we attempt to begin writeback. The exception is subpage metadata writes, where we know we don't have an eb under writeback and we're doing it one eb at a time. Since b5612c368648 ("mm: return void from folio_start_writeback() and related functions") we now will BUG_ON() if we call folio_start_writeback() on a folio that's already under writeback. Previously folio_start_writeback() would bail if writeback was already started. Fix this in the subpage code by checking if we have writeback set and skipping it if we do. This fixes the panic we were seeing on subpage. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/subpage.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)