Re: [PATCH 04/42] btrfs: introduce submit_eb_subpage() to submit a subpage metadata page

[Josef Bacik <josef@toxicpanda.com>]

On 4/15/21 7:28 PM, Qu Wenruo wrote:
> 
> 
> On 2021/4/16 上午3:27, Josef Bacik wrote:
>> On 4/15/21 1:04 AM, Qu Wenruo wrote:
>>> The new function, submit_eb_subpage(), will submit all the dirty extent
>>> buffers in the page.
>>>
>>> The major difference between submit_eb_page() and submit_eb_subpage()
>>> is:
>>> - How to grab extent buffer
>>>    Now we use find_extent_buffer_nospinlock() other than using
>>>    page::private.
>>>
>>> All other different handling is already done in functions like
>>> lock_extent_buffer_for_io() and write_one_eb().
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>   fs/btrfs/extent_io.c | 95 ++++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 95 insertions(+)
>>>
>>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>>> index c068c2fcba09..7d1fca9b87f0 100644
>>> --- a/fs/btrfs/extent_io.c
>>> +++ b/fs/btrfs/extent_io.c
>>> @@ -4323,6 +4323,98 @@ static noinline_for_stack int
>>> write_one_eb(struct extent_buffer *eb,
>>>       return ret;
>>>   }
>>> +/*
>>> + * Submit one subpage btree page.
>>> + *
>>> + * The main difference between submit_eb_page() is:
>>> + * - Page locking
>>> + *   For subpage, we don't rely on page locking at all.
>>> + *
>>> + * - Flush write bio
>>> + *   We only flush bio if we may be unable to fit current extent
>>> buffers into
>>> + *   current bio.
>>> + *
>>> + * Return >=0 for the number of submitted extent buffers.
>>> + * Return <0 for fatal error.
>>> + */
>>> +static int submit_eb_subpage(struct page *page,
>>> +                 struct writeback_control *wbc,
>>> +                 struct extent_page_data *epd)
>>> +{
>>> +    struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
>>> +    int submitted = 0;
>>> +    u64 page_start = page_offset(page);
>>> +    int bit_start = 0;
>>> +    int nbits = BTRFS_SUBPAGE_BITMAP_SIZE;
>>> +    int sectors_per_node = fs_info->nodesize >>
>>> fs_info->sectorsize_bits;
>>> +    int ret;
>>> +
>>> +    /* Lock and write each dirty extent buffers in the range */
>>> +    while (bit_start < nbits) {
>>> +        struct btrfs_subpage *subpage = (struct btrfs_subpage
>>> *)page->private;
>>> +        struct extent_buffer *eb;
>>> +        unsigned long flags;
>>> +        u64 start;
>>> +
>>> +        /*
>>> +         * Take private lock to ensure the subpage won't be detached
>>> +         * halfway.
>>> +         */
>>> +        spin_lock(&page->mapping->private_lock);
>>> +        if (!PagePrivate(page)) {
>>> +            spin_unlock(&page->mapping->private_lock);
>>> +            break;
>>> +        }
>>> +        spin_lock_irqsave(&subpage->lock, flags);
>>
>> writepages doesn't get called with irq context, so you can just do
>> spin_lock_irq()/spin_unlock_irq().
> 
> But this spinlock is used in endio function.
> If we don't use irqsave variant here, won't an endio interruption call
> sneak in and screw up everything?
> 

No, you use irqsave if the function can be called under irq.  So in the endio 
call you do irqsave.  This function can't be called by an interrupt handler, so 
just _irq() is fine because you just need to disable irq's.  Thanks,

Josef