Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 2/2] erofs: add sysfs node to control sync decompression
 strategy
To:     Gao Xiang <hsiangkao@linux.alibaba.com>
Cc:     linux-erofs@lists.ozlabs.org, zhangshiming@oppo.com,
        linux-kernel@vger.kernel.org, yh@oppo.com, guanyuwei@oppo.com,
        guoweichao@oppo.com
References: <20211109025445.12427-1-huangjianan@oppo.com>
 <20211109025445.12427-2-huangjianan@oppo.com>
 <YYnoHw+boVFtcyfv@B-P7TQMD6M-0146.local>
From:   Huang Jianan <huangjianan@oppo.com>
Message-ID: <82f7c99e-b83f-90b7-fceb-b8436da94339@oppo.com>
Date:   Tue, 9 Nov 2021 15:43:06 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
In-Reply-To: <YYnoHw+boVFtcyfv@B-P7TQMD6M-0146.local>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NldCR0dLc1JOcG1sNWpkMzlyamVLbmxxcnFJZVlQOWNHT1N3MjZlVDU5VE5E?=
 =?utf-8?B?UHRQWWtCZXovaXZuRFlyWFI2bUdGb0lTc09LKytiRDJIbzdUSFQrWHZSUjc3?=
 =?utf-8?B?MEZOYU9hSVBrRkwxYndGMFdyQ1pIWnpVYjhRazNIaHB1OEtwQzZZaGsrVGZQ?=
 =?utf-8?B?REI4eStlcVZjYzFRYlJyNGxaVXc4VkhVTlNrODZDaHdjSFV1amlTYi9vRTVI?=
 =?utf-8?B?UmRpWkh6MzNDc2VnOWowSnlha0pXMVBYd0F2OTJGUUs5blF0Nld2YWlWbjgv?=
 =?utf-8?B?VVJydnAyQTFaR3EzRk9KcUhPUnQxZThNQXlFZDFkSGJpdHFGZEEreVJ5STha?=
 =?utf-8?B?bnppUnpYMVJDam93QktaTHd4UjNFR0JTNThpZTBPWlJCVURjS0pDdXlwN3FQ?=
 =?utf-8?B?Y2RHUUNHaDRKZmNSSEhvN094Yjc2elY5aC8vM2tnZno4WjlJSHRxOFY3WVBk?=
 =?utf-8?B?MWl1TXJGbFk0NE05OFhVTXNNT2JJWVhtdXU2Nmw0dHJkVm11NDNCKytUazho?=
 =?utf-8?B?LzJGSDkyZ3JSbE1hOE03eThNV1BZTDB2YnAzeVZwejQrMmVDN0ZoSFBYZlpB?=
 =?utf-8?B?cVhIQzhIUGNBMHUrUVdqQ05mc2VlYlhhdUlzam9PaWo3ZmFJOTYvRWR5M0ho?=
 =?utf-8?B?aDNabjFOaFphdVA0V1VLdXRhbVpTdnhoR0Q0WCs4KzR1Q3pScXRBWDA3ajM1?=
 =?utf-8?B?U1NGVlRFZC9nWGowNTlmaTM1Qy9VQVVxMEVXK203S2NUdUg4YlN4Ymo5eXJp?=
 =?utf-8?B?a2ZJRERoRmtVS3BRb1FLOFBiS3BHTFQ5ekhZbGpBSnl2ak10YU5kZ2JJRzVr?=
 =?utf-8?B?eDhhWjlRRTZ4elJsVVlmV3pJRzBuM3VtYXE4L3R5ZW1qLzJONzkwVEZubmQz?=
 =?utf-8?B?cGNTTEdKeEg1MGpmWitGWmRlaVhnaEhDaUE5NEx2UGIyYnZ2WE5KTUc2TnBN?=
 =?utf-8?B?Rm8wTWZLLzlDK1luRXRFSU1VYk5tNm5BN0tqTFpmeklwbERBNDA3bVJJclFF?=
 =?utf-8?B?N2tMSGp2bGZUb3QxWC93T2VGaUpUOFpDOUc1UmRTWFBQSVFyNWlnWDhRWmFR?=
 =?utf-8?B?eURNbGhQdWxra2xWeGZqWjE4YXgxSk82Qm4rd1h0ODZsRUs5enhaZ3RnelNI?=
 =?utf-8?B?QXVyLzgwUmdJR1N3b1p4bTU1c1J5SkRpZlNWdW9JdTdxY0tQUlRhNzVWczd0?=
 =?utf-8?B?dm1zRlNRd1NaYVdybmdYMlYxcm5sOGg0WTg0dTZsbmh2U0RtZHlpS0hZempL?=
 =?utf-8?B?N2JqUGFPZjM3WmlWWnF0d0puV01vdXRCUTJ1YmtiOUFGL2pydUpjc2QzajVv?=
 =?utf-8?B?OGtUWWxBNTJ3SGxHeHU5bmwrcUlVdVZiZXJlL0hEWVVTRkRqQ2UzUDgvZlBs?=
 =?utf-8?B?ZDhVZVUybWFLbENFRE9ldTNlaXpDRFMwaHdXWnI4WXlQNDFBSzhTR3UwRjkv?=
 =?utf-8?B?clM4c04yM1gzamxBMTBJT2ZBd1R1QXFMaWhtcjRVWnhoSDlUNWxYVzZsMFMw?=
 =?utf-8?B?VG8yMU1HUjVkTUwvanFLYkRXOWdEbzZnZVdrM1Z1cmY3VStLbVZHTFZaRTJ2?=
 =?utf-8?B?SGY0a2NMREFwdWROMFd2aGpNVDUrQTdIT2VzZEw4MERwa1pvMGNDVUM3SmdJ?=
 =?utf-8?B?RU5zc214TEJxTUhmQ29XZnNOdDZSKzJ3R0dka3Nxa3lqKzNmRFI0M1hkZXNo?=
 =?utf-8?B?alEycGtXNkw3U2xucC9PMHJJWVp2bXNPMWpMZFIvRWlrSUsySzFBVXhLbVIz?=
 =?utf-8?B?OGk5V3pZcGlaRk9HTFVEQTlsM2h4a3JnOEtDcjBSdUlEYk9GNWsrUk12dVV5?=
 =?utf-8?B?ZlA1ZW1DM3UzZVd6UERkQzdUbGkzbTkxTisvaERodklHckEreThRUlI3WHNT?=
 =?utf-8?B?aTJjZ3ZVSUNwZHlwRkFPNFZ4Q2pOU1lIUm9EVEl0VDl3RzZsUERvQnJLYmh4?=
 =?utf-8?B?OGVxNlhPekdSSkVQQ08wUHJhRG9rR2hZbGt4eHRqbFVrYTBjMlBVLzVjMmYw?=
 =?utf-8?B?MFZnS2RKbzlaU1RIbG5JTVVGeldIS3dBcTh1dXYydmI5aTh2cVBhWHdDVkpL?=
 =?utf-8?B?K3NxT2IrWUxGQ3B2amVZTG9kenRBR0oyVUJESHV3T3FvMURiZHlsS3M3ekRM?=
 =?utf-8?B?RkxmRzBNRmN6bVYwYzVWZi93UmZVS0duVlBQd3NHZUVRZTEwYkora0EwMXVo?=
 =?utf-8?Q?w0s5EBAlVHlo0OAtconWeZQ=3D?=
X-OriginatorOrg: oppo.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a82acf49-6d42-414a-ffc9-08d9a3549334
X-MS-Exchange-CrossTenant-AuthSource: SG2PR02MB4108.apcprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Nov 2021 07:43:09.2819
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: f1905eb1-c353-41c5-9516-62b4a54b5ee6
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: aGIItg4I9tTVhHzoeaaZEwZC9OiG7XNaCmKNtEpGC1I+Biq0BtNEbGhT7gG70SAsDCTjVES95zEkh2eKZ6joIg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SG2PR02MB4024
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Xiang,

在 2021/11/9 11:16, Gao Xiang 写道:
> On Tue, Nov 09, 2021 at 10:54:45AM +0800, Huang Jianan via Linux-erofs wrote:
>> Although readpage is a synchronous path, there will be no additional
>> kworker scheduling overhead in non-atomic contexts. So we can add a
>> sysfs node to allow disable sync decompression.
>>
>> Signed-off-by: Huang Jianan <huangjianan@oppo.com>
>> ---
>>   fs/erofs/internal.h | 2 +-
>>   fs/erofs/super.c    | 2 +-
>>   fs/erofs/sysfs.c    | 6 ++++++
>>   fs/erofs/zdata.c    | 9 ++++-----
>>   4 files changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
>> index d0cd712dc222..1ab96878009d 100644
>> --- a/fs/erofs/internal.h
>> +++ b/fs/erofs/internal.h
>> @@ -60,7 +60,7 @@ struct erofs_mount_opts {
>>   #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) */
>> +	/* strategy of sync decompression (false - disable, true - force on) */
> Please leave false - auto.
Agree.
>>   	bool readahead_sync_decompress;
>>   
>>   	/* threshold for decompression synchronously */
>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
>> index abc1da5d1719..26585d865503 100644
>> --- a/fs/erofs/super.c
>> +++ b/fs/erofs/super.c
>> @@ -423,7 +423,7 @@ static void erofs_default_options(struct erofs_fs_context *ctx)
>>   #ifdef CONFIG_EROFS_FS_ZIP
>>   	ctx->opt.cache_strategy = EROFS_ZIP_CACHE_READAROUND;
>>   	ctx->opt.max_sync_decompress_pages = 3;
>> -	ctx->opt.readahead_sync_decompress = false;
>> +	ctx->opt.readahead_sync_decompress = true;
> Please leave readahead_sync_decompress = false 'auto' by default.
>
> I don't like .readpage() applies async decompression since it's
> actually a sync way, otherwise, it will cause more scheduling
> overhead, see:
> https://lore.kernel.org/r/20201016160443.18685-1-willy@infradead.org
> https://lore.kernel.org/r/20201102184312.25926-16-willy@infradead.org

Maybe consider adding a disbale strategy? And then we will have:
0 - auto, 1 - force on, 2 - disable
I will try this in the next version.

for .readpage(), If the decompression can be done in a non-atomic context,
there will be no additional scheduling overhead? In this case, we can 
turn off
the syncdecompression.

Thanks,
Jianan

> Thanks,
> Gao Xiang