linux-erofs.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
From: Huang Jianan via Linux-erofs <linux-erofs@lists.ozlabs.org>
To: Chao Yu <yuchao0@huawei.com>, linux-erofs@lists.ozlabs.org
Cc: zhangshiming@oppo.com, guoweichao@oppo.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6 2/2] erofs: decompress in endio if possible
Date: Wed, 17 Mar 2021 11:54:15 +0800	[thread overview]
Message-ID: <d763d340-a982-c56f-26b5-5c7045301c5b@oppo.com> (raw)
In-Reply-To: <cb51e5de-bfb9-509d-e165-243157404cb9@huawei.com>


On 2021/3/16 16:26, Chao Yu wrote:
> Hi Jianan,
>
> On 2021/3/16 11:15, Huang Jianan via Linux-erofs wrote:
>> z_erofs_decompressqueue_endio may not be executed in the atomic
>> context, for example, when dm-verity is turned on. In this scenario,
>> data can be decompressed directly to get rid of additional kworker
>> scheduling overhead. Also, it makes no sense to apply synchronous
>> decompression for such case.
>
> It looks this patch does more than one things:
> - combine dm-verity and erofs workqueue
> - change policy of decompression in context of thread
>
> Normally, we do one thing in one patch, by this way, we will be 
> benefit in
> scenario of when backporting patches and bisecting problematic patch with
> minimum granularity, and also it will help reviewer to focus on reviewing
> single code logic by following patch's goal.
>
> So IMO, it would be better to separate this patch into two.
>
Thanks for the suggestion, I will send a new patch set.
> One more thing is could you explain a little bit more about why we 
> need to
> change policy of decompression in context of thread? for better 
> performance?
>
Sync decompression was introduced to get rid of additional kworker 
scheduling

overhead. But there is no such overhead in if we try to decompress 
directly in

z_erofs_decompressqueue_endio . Therefore, it  should be better to turn off

sync decompression to avoid the current thread waiting in z_erofs_runqueue.

> BTW, code looks clean to me. :)
>
> Thanks,
>
>>
>> Signed-off-by: Huang Jianan <huangjianan@oppo.com>
>> Signed-off-by: Guo Weichao <guoweichao@oppo.com>
>> Reviewed-by: Gao Xiang <hsiangkao@redhat.com>
>> ---
>>   fs/erofs/internal.h |  2 ++
>>   fs/erofs/super.c    |  1 +
>>   fs/erofs/zdata.c    | 15 +++++++++++++--
>>   3 files changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
>> index 67a7ec945686..fbc4040715be 100644
>> --- a/fs/erofs/internal.h
>> +++ b/fs/erofs/internal.h
>> @@ -50,6 +50,8 @@ struct erofs_fs_context {
>>   #ifdef CONFIG_EROFS_FS_ZIP
>>       /* current strategy of how to use managed cache */
>>       unsigned char cache_strategy;
>> +    /* strategy of sync decompression (false - auto, true - force 
>> on) */
>> +    bool readahead_sync_decompress;
>>         /* threshold for decompression synchronously */
>>       unsigned int max_sync_decompress_pages;
>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
>> index d5a6b9b888a5..0445d09b6331 100644
>> --- a/fs/erofs/super.c
>> +++ b/fs/erofs/super.c
>> @@ -200,6 +200,7 @@ static void erofs_default_options(struct 
>> erofs_fs_context *ctx)
>>   #ifdef CONFIG_EROFS_FS_ZIP
>>       ctx->cache_strategy = EROFS_ZIP_CACHE_READAROUND;
>>       ctx->max_sync_decompress_pages = 3;
>> +    ctx->readahead_sync_decompress = false;
>>   #endif
>>   #ifdef CONFIG_EROFS_FS_XATTR
>>       set_opt(ctx, XATTR_USER);
>> diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
>> index 6cb356c4217b..25a0c4890d0a 100644
>> --- a/fs/erofs/zdata.c
>> +++ b/fs/erofs/zdata.c
>> @@ -706,9 +706,12 @@ static int z_erofs_do_read_page(struct 
>> z_erofs_decompress_frontend *fe,
>>       goto out;
>>   }
>>   +static void z_erofs_decompressqueue_work(struct work_struct *work);
>>   static void z_erofs_decompress_kickoff(struct 
>> z_erofs_decompressqueue *io,
>>                          bool sync, int bios)
>>   {
>> +    struct erofs_sb_info *const sbi = EROFS_SB(io->sb);
>> +
>>       /* wake up the caller thread for sync decompression */
>>       if (sync) {
>>           unsigned long flags;
>> @@ -720,8 +723,15 @@ static void z_erofs_decompress_kickoff(struct 
>> z_erofs_decompressqueue *io,
>>           return;
>>       }
>>   -    if (!atomic_add_return(bios, &io->pending_bios))
>> +    if (atomic_add_return(bios, &io->pending_bios))
>> +        return;
>> +    /* Use workqueue and sync decompression for atomic contexts only */
>> +    if (in_atomic() || irqs_disabled()) {
>>           queue_work(z_erofs_workqueue, &io->u.work);
>> +        sbi->ctx.readahead_sync_decompress = true;
>> +        return;
>> +    }
>> +    z_erofs_decompressqueue_work(&io->u.work);
>>   }
>>     static bool z_erofs_page_is_invalidated(struct page *page)
>> @@ -1333,7 +1343,8 @@ static void z_erofs_readahead(struct 
>> readahead_control *rac)
>>       struct erofs_sb_info *const sbi = EROFS_I_SB(inode);
>>         unsigned int nr_pages = readahead_count(rac);
>> -    bool sync = (nr_pages <= sbi->ctx.max_sync_decompress_pages);
>> +    bool sync = (sbi->ctx.readahead_sync_decompress &&
>> +            nr_pages <= sbi->ctx.max_sync_decompress_pages);
>>       struct z_erofs_decompress_frontend f = 
>> DECOMPRESS_FRONTEND_INIT(inode);
>>       struct page *page, *head = NULL;
>>       LIST_HEAD(pagepool);
>>

  reply	other threads:[~2021-03-17  3:54 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-16  3:15 [PATCH v6 1/2] erofs: avoid memory allocation failure during rolling decompression Huang Jianan via Linux-erofs
2021-03-16  3:15 ` [PATCH v6 2/2] erofs: decompress in endio if possible Huang Jianan via Linux-erofs
2021-03-16  8:26   ` Chao Yu
2021-03-17  3:54     ` Huang Jianan via Linux-erofs [this message]
2021-03-16  7:38 ` [PATCH v6 1/2] erofs: avoid memory allocation failure during rolling decompression Chao Yu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d763d340-a982-c56f-26b5-5c7045301c5b@oppo.com \
    --to=linux-erofs@lists.ozlabs.org \
    --cc=guoweichao@oppo.com \
    --cc=huangjianan@oppo.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=yuchao0@huawei.com \
    --cc=zhangshiming@oppo.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).