* [PATCH 00/21] Lock extents before pages
@ 2023-03-02 22:24 Goldwyn Rodrigues
2023-03-09 18:10 ` David Sterba
0 siblings, 1 reply; 6+ messages in thread
From: Goldwyn Rodrigues @ 2023-03-02 22:24 UTC (permalink / raw)
To: linux-btrfs
The main idea is that instead of handling extent locks per page, lock
the extents before handling the pages. This will help in calling iomap
functions from within extent locks, so all page/folio handling is
performed by iomap code.
The patch "debug extent locking" was added to debug this code to check
where the locks were initiated in case of deadlock. No need to add
this patch.
Changes since v1:
- qgroup flush reversal, instead pass nowait=true and handle
qgroup flush if qgroup reservations return EDQUOT
- relocation - locking extents before pages
- async code locking fix: async code deadlocked because parallel
writebacks could set HAS_ASYNC_EXTENTS flag while other thread was
performing a sync writeback. This required re-writing the async writeback
path
- incorporated lock range checks using WARN_ON(), though I must
admit locking during truncate still looks ugly
- added readahead_begin() for locking extents before calling readahead()
since readahead is called with pages locked.
Goldwyn Rodrigues (21):
fs: readahead_begin() to call before locking folio
btrfs: add WARN_ON() on incorrect lock range
btrfs: Add start < end check in btrfs_debug_check_extent_io_range()
btrfs: make btrfs_qgroup_flush() non-static
btrfs: Lock extents before pages for buffered write()
btrfs: wait ordered range before locking during truncate
btrfs: lock extents while truncating
btrfs: no need to lock extent while performing invalidate_folio()
btrfs: lock extents before folio for read()s
btrfs: lock extents before pages in writepages
btrfs: locking extents for async writeback
btrfs: lock extents before pages - defrag
btrfs: Perform memory faults under locked extent
btrfs: writepage fixup lock rearrangement
btrfs: lock extent before pages for encoded read ioctls
btrfs: lock extent before pages in encoded write
btrfs: btree_writepages lock extents before pages
btrfs: check if writeback pages exist before starting writeback
btrfs: lock extents before pages in relocation
btrfs: Add inode->i_count instead of calling ihold()
btrfs: debug extent locking
fs/btrfs/compression.c | 9 +-
fs/btrfs/defrag.c | 48 +---
fs/btrfs/disk-io.c | 28 ++-
fs/btrfs/extent-io-tree.c | 32 +--
fs/btrfs/extent-io-tree.h | 46 ++--
fs/btrfs/extent_io.c | 74 +-----
fs/btrfs/extent_io.h | 2 +
fs/btrfs/extent_map.c | 2 +-
fs/btrfs/file.c | 100 +++-----
fs/btrfs/free-space-cache.c | 2 +-
fs/btrfs/inode.c | 431 ++++++++++++++++++-----------------
fs/btrfs/ordered-data.c | 8 +-
fs/btrfs/ordered-data.h | 3 +-
fs/btrfs/qgroup.c | 6 +-
fs/btrfs/qgroup.h | 1 +
fs/btrfs/relocation.c | 44 ++--
include/linux/fs.h | 1 +
include/trace/events/btrfs.h | 18 +-
mm/readahead.c | 3 +
19 files changed, 397 insertions(+), 461 deletions(-)
--
2.39.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 00/21] Lock extents before pages
2023-03-02 22:24 [PATCH 00/21] Lock extents before pages Goldwyn Rodrigues
@ 2023-03-09 18:10 ` David Sterba
2023-03-10 7:50 ` Christoph Hellwig
0 siblings, 1 reply; 6+ messages in thread
From: David Sterba @ 2023-03-09 18:10 UTC (permalink / raw)
To: Goldwyn Rodrigues; +Cc: linux-btrfs
On Thu, Mar 02, 2023 at 04:24:45PM -0600, Goldwyn Rodrigues wrote:
> The main idea is that instead of handling extent locks per page, lock
> the extents before handling the pages. This will help in calling iomap
> functions from within extent locks, so all page/folio handling is
> performed by iomap code.
>
> The patch "debug extent locking" was added to debug this code to check
> where the locks were initiated in case of deadlock. No need to add
> this patch.
>
> Changes since v1:
> - qgroup flush reversal, instead pass nowait=true and handle
> qgroup flush if qgroup reservations return EDQUOT
> - relocation - locking extents before pages
> - async code locking fix: async code deadlocked because parallel
> writebacks could set HAS_ASYNC_EXTENTS flag while other thread was
> performing a sync writeback. This required re-writing the async writeback
> path
> - incorporated lock range checks using WARN_ON(), though I must
> admit locking during truncate still looks ugly
> - added readahead_begin() for locking extents before calling readahead()
> since readahead is called with pages locked.
>
> Goldwyn Rodrigues (21):
> fs: readahead_begin() to call before locking folio
> btrfs: add WARN_ON() on incorrect lock range
> btrfs: Add start < end check in btrfs_debug_check_extent_io_range()
> btrfs: make btrfs_qgroup_flush() non-static
> btrfs: Lock extents before pages for buffered write()
> btrfs: wait ordered range before locking during truncate
> btrfs: lock extents while truncating
> btrfs: no need to lock extent while performing invalidate_folio()
> btrfs: lock extents before folio for read()s
> btrfs: lock extents before pages in writepages
> btrfs: locking extents for async writeback
> btrfs: lock extents before pages - defrag
> btrfs: Perform memory faults under locked extent
> btrfs: writepage fixup lock rearrangement
> btrfs: lock extent before pages for encoded read ioctls
> btrfs: lock extent before pages in encoded write
> btrfs: btree_writepages lock extents before pages
> btrfs: check if writeback pages exist before starting writeback
> btrfs: lock extents before pages in relocation
> btrfs: Add inode->i_count instead of calling ihold()
> btrfs: debug extent locking
I've picked the branch from your git repository (applies on top of
misc-next) and will add it to for-next. We may need to keep it there for
some time, with this kind of core change it's not possible to do reverts
in case we find serious problems.
Optimistic plan is to get it stabilized during current dev cycle and
then one more cycle for stabilization once it's in the master branch.
A general comment for all patches is that the changes need more
explanation regarding the effects of the reversed locking for the
particular context. With the longer time span for the merge we can add
it as we find bugs or need to clarify things.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 00/21] Lock extents before pages
2023-03-09 18:10 ` David Sterba
@ 2023-03-10 7:50 ` Christoph Hellwig
2023-03-10 16:38 ` Goldwyn Rodrigues
2023-03-15 19:16 ` David Sterba
0 siblings, 2 replies; 6+ messages in thread
From: Christoph Hellwig @ 2023-03-10 7:50 UTC (permalink / raw)
To: David Sterba; +Cc: Goldwyn Rodrigues, linux-btrfs
On Thu, Mar 09, 2023 at 07:10:51PM +0100, David Sterba wrote:
> I've picked the branch from your git repository (applies on top of
> misc-next) and will add it to for-next. We may need to keep it there for
> some time, with this kind of core change it's not possible to do reverts
> in case we find serious problems.
I am really worried about the i_count hack. It is fundamentlly wrong
and breaks inode lifetime rules. Furthermore there is no attempt
at understanding why it happens (or even if it is new with this series).
>
> Optimistic plan is to get it stabilized during current dev cycle and
> then one more cycle for stabilization once it's in the master branch.
It is going to clash with quite a bit of other work going in this area
I fear.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 00/21] Lock extents before pages
2023-03-10 7:50 ` Christoph Hellwig
@ 2023-03-10 16:38 ` Goldwyn Rodrigues
2023-03-13 9:16 ` Christoph Hellwig
2023-03-15 19:16 ` David Sterba
1 sibling, 1 reply; 6+ messages in thread
From: Goldwyn Rodrigues @ 2023-03-10 16:38 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: David Sterba, linux-btrfs, Josef Bacik
On 23:50 09/03, Christoph Hellwig wrote:
> On Thu, Mar 09, 2023 at 07:10:51PM +0100, David Sterba wrote:
> > I've picked the branch from your git repository (applies on top of
> > misc-next) and will add it to for-next. We may need to keep it there for
> > some time, with this kind of core change it's not possible to do reverts
> > in case we find serious problems.
>
> I am really worried about the i_count hack. It is fundamentlly wrong
> and breaks inode lifetime rules. Furthermore there is no attempt
> at understanding why it happens (or even if it is new with this series).
So this (another version) patch which first added working with i_count is:
8180ef8894fa ("Btrfs: keep inode pinned when compressing writes")
For async, inode is still referenced until all the pages
are written back and cleared in end_compressed_writeback().
evict() waits for all writeback to be completed. Josef, is this
correct?
I removed the ihold() and delayed iput sequence and shifted
unlock_extent() immediately after submit_compressed_extents(). It is
working fine with no crashes for compress tests. But since this is a
race condition I need to be sure if it is the correct thing to do. I
have updated the git [1] but it needs more testing.
In any case, there is another issue with scrubbing which I need to
resolve: btrfs/06[0-3]. I am on vacation next week and the earliest I
can get back to it is March 20.
>
> >
> > Optimistic plan is to get it stabilized during current dev cycle and
> > then one more cycle for stabilization once it's in the master branch.
>
> It is going to clash with quite a bit of other work going in this area
> I fear.
[1] https://github.com/goldwynr/linux/tree/lock-rearrange
--
Goldwyn
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 00/21] Lock extents before pages
2023-03-10 16:38 ` Goldwyn Rodrigues
@ 2023-03-13 9:16 ` Christoph Hellwig
0 siblings, 0 replies; 6+ messages in thread
From: Christoph Hellwig @ 2023-03-13 9:16 UTC (permalink / raw)
To: Goldwyn Rodrigues
Cc: Christoph Hellwig, David Sterba, linux-btrfs, Josef Bacik
On Fri, Mar 10, 2023 at 10:38:28AM -0600, Goldwyn Rodrigues wrote:
> > I am really worried about the i_count hack. It is fundamentlly wrong
> > and breaks inode lifetime rules. Furthermore there is no attempt
> > at understanding why it happens (or even if it is new with this series).
>
> So this (another version) patch which first added working with i_count is:
> 8180ef8894fa ("Btrfs: keep inode pinned when compressing writes")
>
> For async, inode is still referenced until all the pages
> are written back and cleared in end_compressed_writeback().
> evict() waits for all writeback to be completed. Josef, is this
> correct?
>
> I removed the ihold() and delayed iput sequence and shifted
> unlock_extent() immediately after submit_compressed_extents(). It is
> working fine with no crashes for compress tests. But since this is a
> race condition I need to be sure if it is the correct thing to do. I
> have updated the git [1] but it needs more testing.
I think the import thing to remember here is:
1) an inode is not getting fred as long as it has pages under writeback
2) as long as you don't reference the inode after clearing the last
PageWriteback bit, using the inode without an extra reference
in the writeback code is fine
So I think we can just remove the extra referene after making sure that
the inode is not referenced after dropping the last writeback bit.
And handling of the writeback bit is quite funny in btrfs in a few
places. Maybe I'll be able to find some time to dig into it while
you're away.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 00/21] Lock extents before pages
2023-03-10 7:50 ` Christoph Hellwig
2023-03-10 16:38 ` Goldwyn Rodrigues
@ 2023-03-15 19:16 ` David Sterba
1 sibling, 0 replies; 6+ messages in thread
From: David Sterba @ 2023-03-15 19:16 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: David Sterba, Goldwyn Rodrigues, linux-btrfs
On Thu, Mar 09, 2023 at 11:50:31PM -0800, Christoph Hellwig wrote:
> On Thu, Mar 09, 2023 at 07:10:51PM +0100, David Sterba wrote:
> > I've picked the branch from your git repository (applies on top of
> > misc-next) and will add it to for-next. We may need to keep it there for
> > some time, with this kind of core change it's not possible to do reverts
> > in case we find serious problems.
>
> I am really worried about the i_count hack. It is fundamentlly wrong
> and breaks inode lifetime rules. Furthermore there is no attempt
> at understanding why it happens (or even if it is new with this series).
I don't think the i_count hack will stay until the final merge.
> > Optimistic plan is to get it stabilized during current dev cycle and
> > then one more cycle for stabilization once it's in the master branch.
>
> It is going to clash with quite a bit of other work going in this area
> I fear.
Yeah so we'll need to decide order of merging. The locking change is
needed for iomap conversion and we need to do it at some point, knowing
that it's a risky change. Limiting amout of work in the same area is a
good thing also because of testing.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-03-15 19:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-02 22:24 [PATCH 00/21] Lock extents before pages Goldwyn Rodrigues
2023-03-09 18:10 ` David Sterba
2023-03-10 7:50 ` Christoph Hellwig
2023-03-10 16:38 ` Goldwyn Rodrigues
2023-03-13 9:16 ` Christoph Hellwig
2023-03-15 19:16 ` David Sterba
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.