| Message ID | 6c59a12446b7583172c886bee886d5229f7dccd5.1602499588.git.fdmanana@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: some readhead fixes after removing or replacing a device | expand |
On 12/10/2020 12:55, fdmanana@kernel.org wrote: > critical section delimited by this lock, while all other places that are > sure they are not dropping the last reference, do not bother calling > kref_put() while holding that lock. Quick question for me to understand this change, is 'reada_pick_zone' the place that's certain it doesn't put the last reference? If yes we should maybe add a comment in there.
On Tue, Oct 13, 2020 at 10:08 AM Johannes Thumshirn <Johannes.Thumshirn@wdc.com> wrote: > > On 12/10/2020 12:55, fdmanana@kernel.org wrote: > > critical section delimited by this lock, while all other places that are > > sure they are not dropping the last reference, do not bother calling > > kref_put() while holding that lock. > > Quick question for me to understand this change, is 'reada_pick_zone' the place > that's certain it doesn't put the last reference? All places that don't take the lock before calling kref_put() are the places that don't drop the last reference, and there are several - reada_find_extent(), reada_extent_put() and reada_pick_zone() at least. As far as I could see, all are correct. The change is motivated because I had an earlier fix that did the final kref_put() without holding the lock. Thanks. > > If yes we should maybe add a comment in there. >
On 13/10/2020 11:15, Filipe Manana wrote: > On Tue, Oct 13, 2020 at 10:08 AM Johannes Thumshirn > <Johannes.Thumshirn@wdc.com> wrote: >> >> On 12/10/2020 12:55, fdmanana@kernel.org wrote: >>> critical section delimited by this lock, while all other places that are >>> sure they are not dropping the last reference, do not bother calling >>> kref_put() while holding that lock. >> >> Quick question for me to understand this change, is 'reada_pick_zone' the place >> that's certain it doesn't put the last reference? > > All places that don't take the lock before calling kref_put() are the > places that don't drop the last reference, and there are several - > reada_find_extent(), reada_extent_put() and reada_pick_zone() at > least. > As far as I could see, all are correct. The change is motivated > because I had an earlier fix that did the final kref_put() without > holding the lock. > Am I looking at the wrong tree? reada_find_extent() takes the lock: fs/btrfs/reada.c- spin_lock(&fs_info->reada_lock); fs/btrfs/reada.c: kref_put(&zone->refcnt, reada_zone_release); fs/btrfs/reada.c- spin_unlock(&fs_info->reada_lock); and so does reada_extent_put: fs/btrfs/reada.c- for (i = 0; i < re->nzones; ++i) { fs/btrfs/reada.c- struct reada_zone *zone = re->zones[i]; fs/btrfs/reada.c- fs/btrfs/reada.c- kref_get(&zone->refcnt); fs/btrfs/reada.c- spin_lock(&zone->lock); fs/btrfs/reada.c- --zone->elems; fs/btrfs/reada.c- if (zone->elems == 0) { fs/btrfs/reada.c- /* no fs_info->reada_lock needed, as this can't be fs/btrfs/reada.c- * the last ref */ fs/btrfs/reada.c: kref_put(&zone->refcnt, reada_zone_release); fs/btrfs/reada.c- } fs/btrfs/reada.c- spin_unlock(&zone->lock); fs/btrfs/reada.c- fs/btrfs/reada.c- spin_lock(&fs_info->reada_lock); fs/btrfs/reada.c: kref_put(&zone->refcnt, reada_zone_release); fs/btrfs/reada.c- spin_unlock(&fs_info->reada_lock);
On 13/10/2020 11:22, Johannes Thumshirn wrote: > fs/btrfs/reada.c- /* no fs_info->reada_lock needed, as this can't be > fs/btrfs/reada.c- * the last ref */ > fs/btrfs/reada.c: kref_put(&zone->refcnt, reada_zone_release); > fs/btrfs/reada.c- } I'm sorry, I'm stupid
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
On 10/12/20 6:55 AM, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > When we drop the last reference of a zone, we end up releasing it through > the callback reada_zone_release(), which deletes the zone from a device's > reada_zones radix tree. This tree is protected by the global readahead > lock at fs_info->reada_lock. Currently all places that are sure that they > are dropping the last reference on a zone, are calling kref_put() in a > critical section delimited by this lock, while all other places that are > sure they are not dropping the last reference, do not bother calling > kref_put() while holding that lock. > > When working on the previous fix for hangs and use-after-frees in the > readahead code, my initial attempts were different and I actually ended > up having reada_zone_release() called when not holding the lock, which > resulted in weird and unexpected problems. So just add an assertion > there to detect such problem more quickly and make the dependency more > obvious. > > Signed-off-by: Filipe Manana <fdmanana@suse.com> Reviewed-by: Josef Bacik <josef@toxicpanda.com> Thanks, Josef
diff --git a/fs/btrfs/reada.c b/fs/btrfs/reada.c index d9a166eb344e..6e33cb755fa5 100644 --- a/fs/btrfs/reada.c +++ b/fs/btrfs/reada.c @@ -531,6 +531,8 @@ static void reada_zone_release(struct kref *kref) { struct reada_zone *zone = container_of(kref, struct reada_zone, refcnt); + lockdep_assert_held(&zone->device->fs_info->reada_lock); + radix_tree_delete(&zone->device->reada_zones, zone->end >> PAGE_SHIFT);