Re: [PATCH 1/2] btrfs: fix wild pointer access during metadata read failure for subpage
On 2021/3/16 上午2:51, David Sterba wrote: On Mon, Mar 15, 2021 at 04:25:32PM +0800, Qu Wenruo wrote: On 2021/3/15 下午3:55, Johannes Thumshirn wrote: On 15/03/2021 06:40, Qu Wenruo wrote: The difference against find_extent_buffer_nospinlock() is: - Also handles regular sectorsize == PAGE_SIZE case - No extent buffer refs increase/decrease As extent buffer under IO must has non-zero refs. Can these be merged into a single function? The sectorsie == PAGE_SIZE case won't do anything for find_extent_buffer_nospinlock() and the atomic_inc_not_zero(&eb->refs) can be hidden behind a 'if (write)' check. That would make the eb refs change too inconsistent. But I get your point. How about calling find_extent_buffer_nospinlock() and then dec the refs manually? Is this equivalent to this patch? Ie. the atomic_inc_not_zero in find_extent_buffer_nospinlock happens inside the RCU section, while what you suggest looks like find_extent_buffer_nospinlock() rcu_lock radix_lookup rcu_unlock atomic_inc_not_zero() No, I mean just call find_extent_buffer_nospinlock() directly. Then manually call atomic_dec() on the returned eb. Thanks, Qu
Re: [PATCH 1/2] btrfs: fix wild pointer access during metadata read failure for subpage
On Mon, Mar 15, 2021 at 04:25:32PM +0800, Qu Wenruo wrote: > > > On 2021/3/15 下午3:55, Johannes Thumshirn wrote: > > On 15/03/2021 06:40, Qu Wenruo wrote: > >> The difference against find_extent_buffer_nospinlock() is: > >> - Also handles regular sectorsize == PAGE_SIZE case > >> - No extent buffer refs increase/decrease > >>As extent buffer under IO must has non-zero refs. > > > > Can these be merged into a single function? The sectorsie == PAGE_SIZE case > > won't do anything for find_extent_buffer_nospinlock() and the > > atomic_inc_not_zero(&eb->refs) can be hidden behind a 'if (write)' check. > > That would make the eb refs change too inconsistent. > > But I get your point. > > How about calling find_extent_buffer_nospinlock() and then dec the refs > manually? Is this equivalent to this patch? Ie. the atomic_inc_not_zero in find_extent_buffer_nospinlock happens inside the RCU section, while what you suggest looks like find_extent_buffer_nospinlock() rcu_lock radix_lookup rcu_unlock atomic_inc_not_zero()
Re: [PATCH 1/2] btrfs: fix wild pointer access during metadata read failure for subpage
On 2021/3/15 下午3:55, Johannes Thumshirn wrote: On 15/03/2021 06:40, Qu Wenruo wrote: The difference against find_extent_buffer_nospinlock() is: - Also handles regular sectorsize == PAGE_SIZE case - No extent buffer refs increase/decrease As extent buffer under IO must has non-zero refs. Can these be merged into a single function? The sectorsie == PAGE_SIZE case won't do anything for find_extent_buffer_nospinlock() and the atomic_inc_not_zero(&eb->refs) can be hidden behind a 'if (write)' check. That would make the eb refs change too inconsistent. But I get your point. How about calling find_extent_buffer_nospinlock() and then dec the refs manually? Thanks, Qu Note, I was looking at this version: https://www.spinics.net/lists/linux-btrfs/msg88.html
Re: [PATCH 1/2] btrfs: fix wild pointer access during metadata read failure for subpage
On 15/03/2021 06:40, Qu Wenruo wrote: > The difference against find_extent_buffer_nospinlock() is: > - Also handles regular sectorsize == PAGE_SIZE case > - No extent buffer refs increase/decrease > As extent buffer under IO must has non-zero refs. Can these be merged into a single function? The sectorsie == PAGE_SIZE case won't do anything for find_extent_buffer_nospinlock() and the atomic_inc_not_zero(&eb->refs) can be hidden behind a 'if (write)' check. Note, I was looking at this version: https://www.spinics.net/lists/linux-btrfs/msg88.html
