Received-SPF: pass (google.com: domain of linux-kernel+bounces-209602-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <f922b3e8-eecb-42c9-8dba-f17f9bdd31b0@oracle.com>
Date: Tue, 11 Jun 2024 11:00:43 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v7 07/11] iomap: fix iomap_dio_zero() for fs bs > system
 page size
To: "Pankaj Raghav (Samsung)" <kernel@pankajraghav.com>
Cc: david@fromorbit.com, djwong@kernel.org, chandan.babu@oracle.com,
        brauner@kernel.org, akpm@linux-foundation.org, willy@infradead.org,
        mcgrof@kernel.org, linux-mm@kvack.org, hare@suse.de,
        linux-kernel@vger.kernel.org, yang@os.amperecomputing.com,
        Zi Yan <zi.yan@sent.com>, linux-xfs@vger.kernel.org,
        p.raghav@samsung.com, linux-fsdevel@vger.kernel.org, hch@lst.de,
        gost.dev@samsung.com, cl@os.amperecomputing.com
References: <20240607145902.1137853-1-kernel@pankajraghav.com>
 <20240607145902.1137853-8-kernel@pankajraghav.com>
 <4c6e092d-5580-42c8-9932-b42995e914be@oracle.com>
 <20240611094137.vxuhldj4b3qslsdj@quentin>
Content-Language: en-US
From: John Garry <john.g.garry@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <20240611094137.vxuhldj4b3qslsdj@quentin>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?NFU5eTlSeUpaOGc5TG12bHdqbUwrS1hHZzhrNnFlb3VjWTFvZGVaa010dVUy?=
 =?utf-8?B?RlBMczhYbnVLL2thYmNidFpsYkkyN05xcDYrNTAweFV4d2pkRmRNOVA1TTJU?=
 =?utf-8?B?bkJQZUlET0RJS0FHaGxXak04ck1MN3JwcWhVRVV6Z3FHRzZTT0hPem5MMXhy?=
 =?utf-8?B?djg3R01Xd0FIUG53NDRSLzh0bmFUdTMxbGtnbjNLN2NxQlJ0SC9Peko4OUcz?=
 =?utf-8?B?T2Q0RnFqTXFNeE95c0FwSVRPVnpjTi9pUHlnOS9MdENraEZxZ0ErQVl4ejI4?=
 =?utf-8?B?VXdnb1Jjd0JTUncwcVROOFBVS2ljNFA2TmQvWFdiWUphaTh2Zjhrb0FaMW9J?=
 =?utf-8?B?dzQ4L2lJTmYrUHgvS25xUTNvZU1oTWgwajdtcHVZcXBJZWtLK1YvWkQ2RXc4?=
 =?utf-8?B?RkxjaGhEcUZMTGtWRXZxa1VaSmQ2ZHFKVStpYUJoek5lUWF2cXNEUElBOXNk?=
 =?utf-8?B?bHpSZWVoSWJac2JpVVd4SktTdlYyUEtMWDByOUQxaE02YlBHcy9FTGMzaXdw?=
 =?utf-8?B?cHl2K0cyWDZQWVJSSW9oNnJ0Q0hETjh3VkMvQzV5Q0djYy9CNVVSdDE1L2JQ?=
 =?utf-8?B?WUx2NG1semozL3lKcWtrVTl3aFhKSC9WYitxbU1VVFl1SlFEd2FtMzhkTytz?=
 =?utf-8?B?Rk94ZDZWSlNlVUs1Y2J0Y3cwRDVHYitDNGJUb2ZuaXZqMkhpeGMrSkFXbFIz?=
 =?utf-8?B?V1M4cXg3WlpSUDhuNEM3N3AzVXNBSU5VVG8vRVVSdHk0eW1ZdDFTN05tNjFS?=
 =?utf-8?B?QjlDVkx2bjVINGNKR0ExaTNxWjM5OWs3QjdtTWNFZjFlbURqSUhFYURCK2k0?=
 =?utf-8?B?QWFHcFFjMkg0TE5EL2tQS2E0cmJ1RENCc3dzWGtGV0NRRVRnVFp4U2FXRzRD?=
 =?utf-8?B?NUxYUHhQTE1zQ21kOFdPSEJaVjAybUE2eFQyVWdrSU5jQlViNE80MUhsaTlX?=
 =?utf-8?B?L3NtSlNDR0FGNDFmWDhLMUw2L2MyVWk5VFNrY0VFcVJTZkRVMklrdlVIektl?=
 =?utf-8?B?UzIrR0VSaFUySFhDYWJnOVVrOEtyMGVBS1JSZDdzVWZ5K0dUR2xhR3Q5K3pW?=
 =?utf-8?B?Z2J2SWkxakROdytDY1ZBNi9TdjNIR01jeGJrcks1d2R1aDBSNGRtK2hqaEd0?=
 =?utf-8?B?NlArWmFhOVFRQzZiSnFSZW9BTVZlM0RDNUFIM0p6YzZMOS91YUlaNjUwSW1k?=
 =?utf-8?B?ayszWlVVUnIxQmlqREJyeDMyd0dReDVFNGNndktiVWQ0cmp1ZGYxSXVQdTBp?=
 =?utf-8?B?TWg4MWd5TEVJMXJBM01uWkRBakdqTitySVI0UVk1WE5RbXVMdXhCRzI1ak9j?=
 =?utf-8?B?MGZkUFQ4MUtpY01VRW1NN1dXdnU3SWRYczNDSys2MTVkanFCYzNlSGwrdnNV?=
 =?utf-8?B?UGdPWkNIRHdaMitzN1FrT2IxTFlOdm54TllCNWVMcHFnbmV2SEZDbGpLYlJU?=
 =?utf-8?B?b0l5K0EzMTUwTmhTMmJaRjd6QldwQ3VzV3JieXNJbWRmc0pKdExKTXRHNmxB?=
 =?utf-8?B?c0Nld3BPZ1p0VTY5SkQ2bTRlQi9lUUJZUExtbXB0SVFVU2NQZ0ljblVWMVdV?=
 =?utf-8?B?V2FhbTJHa29OeXgrVnFlY2M0ZTFGakhteWRUbjFyUzlVUHoyZVY3ZThWVkRR?=
 =?utf-8?B?ZE8zTDlvTWl0cVdiNTdUemluZUZ3YXA1N3RORGtTMmhQWmZnWkxQSGVHenUx?=
 =?utf-8?B?bGQ1SUgxTU5VdVBTVVNkZms2Sm5rYlhSMUdKYXUvYjNLdEJocURveHY5YUFo?=
 =?utf-8?B?bUhqT01jRmVDZWZsR25YYWNCc1hSTXlncnkreW04ekNuYjUzbE1ySjM2VWRK?=
 =?utf-8?B?bXpoR3l6MlQ2RmxmQjJVd0xGaC8yYTdRSXdCY0wzTVZ1dXB3Q055K1kvMWN3?=
 =?utf-8?B?YVpwWEJPWVhOdElBb25EWGpZTVRsb1cxbjdrZ0tuaWtPdDFxa3grcnJmQVNO?=
 =?utf-8?B?MDZWYVBWY0Q2cDloLzZjZ1dsalY3TlRKNGZwUmgxVFZ0QXRNOWZEMHhicnlH?=
 =?utf-8?B?aExrc0NuaFlqdXRySkRtdkV2UE40ZnF1NDY0ZnpBM3J1VVg2M2ZwUHRkSmkx?=
 =?utf-8?B?R0xPUkJ3WUxBQmZTYndybmMxdjBBZWpXZVFqUS9sVm9NN0N2dnFLR1NMbitS?=
 =?utf-8?B?NkErUGlnaXpsYzZFdE56K05JemFzbUVpalhCME54VzExSzFIN0NINnFNdHhp?=
 =?utf-8?B?dEE9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	S8HPbD0mwgwdaqoWzRyAocClSMjnNWH50awCbiA/fYqUYkssW0UudCdhREB4X87pduggHpC8VN4FWJ9PE2hsR1Pm6L8uG5GuGRBllDrnYYHwFBFvZYGjab7SYRaRQBey/FUmNFHVAIJintRd8RUeoD4feNQw3DOG9RzcipkMrfMuoPnyQPSsder2NNlFQfSYtgg7w7Cqug0HqCjaOFijnesBl7wuIpUTtnyqT/Gee86H+X5Vd02SvAl2S7F7udAby/lULEY06P2fXVzvqHL0yxsyZ1l37ssBkXni4n5xhZT6msOEEbR/7d9XoKa5WjDRvyD9OD3A5BhzvcZt73crY2ds0GKrkPh1WvJXpy3qEcfURUWxvNnHU+C1FS0y04WeAjvzvReHRwC0VygefMwkh7ikvpRDwxGKRKnxDzX2Wl9dp/CI5T2rmW+TKTwaSsm140F/29Pijwhdn63xgETajsGKLTaYlz9o2ZFPCm4adWcufEUp0K2XtG3RqxOn87O5DfaxUgyl5pgErUD/EkHLDAi6tSla6SgLGDtJNBoWMPNC5lI+9oUs6UulCfuAaStWZOieX5BMoM6Pagii/zks4G9qQMdaJl5LVdFRMZ+NoV4=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: dffcef7b-d3c2-4f66-d5ef-08dc89fd5f21
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB4313.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Jun 2024 10:00:49.7789
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: ua9b0fy2vOfBb+nNIDDSL1cY28whTBy8eulO/TJA5TdvGxF23f6o6OBOvZKbH5BQ2ohazB1R8bJu1BUPDmTgkw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR10MB7143
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.680,FMLib:17.12.28.16
 definitions=2024-06-11_05,2024-06-11_01,2024-05-17_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 suspectscore=0
 phishscore=0 bulkscore=0 malwarescore=0 spamscore=0 mlxscore=0
 mlxlogscore=999 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2405010000 definitions=main-2406110075
X-Proofpoint-GUID: ntS48cbn-ouqc5gaS09vIqC25Ly8BKq3
X-Proofpoint-ORIG-GUID: ntS48cbn-ouqc5gaS09vIqC25Ly8BKq3

On 11/06/2024 10:41, Pankaj Raghav (Samsung) wrote:
>>> 8419fcc7..9f791db473e4 100644
>>> --- a/fs/iomap/buffered-io.c
>>> +++ b/fs/iomap/buffered-io.c
>>> @@ -1990,6 +1990,12 @@ EXPORT_SYMBOL_GPL(iomap_writepages);
>>>    static int __init iomap_init(void)
>>>    {
>>> +	int ret;
>>> +
>>> +	ret = iomap_dio_init();
>>> +	if (ret)
>>> +		return ret;
>>> +
>>>    	return bioset_init(&iomap_ioend_bioset, 4 * (PAGE_SIZE / SECTOR_SIZE),
>>>    			   offsetof(struct iomap_ioend, io_bio),
>>>    			   BIOSET_NEED_BVECS);
>> I suppose that it does not matter that zero_fs_block is leaked if this fails
>> (or is it even leaked?), as I don't think that failing that bioset_init()
>> call is handled at all.
> If bioset_init fails, then we have even more problems than just a leaked
> 64k memory? ????
> 

Right

 > Do you have something like this in mind?

I wouldn't propose anything myself. AFAICS, we don't gracefully handle 
bioset_init() failing and iomap_ioend_bioset not being initialized properly.

>   static struct bio *iomap_dio_alloc_bio(const struct iomap_iter *iter,
>                  struct iomap_dio *dio, unsigned short nr_vecs, blk_opf_t opf)
>   {
> 
>>> +
>>>    static struct bio *iomap_dio_alloc_bio(const struct iomap_iter *iter,
>>>    		struct iomap_dio *dio, unsigned short nr_vecs, blk_opf_t opf)
>>>    {
>>> @@ -236,17 +253,22 @@ static void iomap_dio_zero(const struct iomap_iter *iter, struct iomap_dio *dio,
>>>    		loff_t pos, unsigned len)
>>>    {
>>>    	struct inode *inode = file_inode(dio->iocb->ki_filp);
>>> -	struct page *page = ZERO_PAGE(0);
>>>    	struct bio *bio;
>>> +	/*
>>> +	 * Max block size supported is 64k
>>> +	 */
>>> +	WARN_ON_ONCE(len > ZERO_FSB_SIZE);
>> JFYI, As mentioned inhttps://urldefense.com/v3/__https://lore.kernel.org/linux-xfs/20240429174746.2132161-1-john.g.garry@oracle.com/T/*m5354e2b2531a5552a8b8acd4a95342ed4d7500f2__;Iw!!ACWV5N9M2RV99hQ!MTwVaC6oueHR_vgmDfOvgBX8bPdeTSRPcRcw5-CqtHnFEH-Ya1sUeZwaF-xrBF5XZ_8lJw5l-riq4t8IkfBhf2Q$  ,
>> we would like to support an arbitrary size. Maybe I will need to loop for
>> zeroing sizes > 64K.
> The initial patches were looping with a ZERO_PAGE(0), but the initial
> feedback was to use a huge zero page. But when I discussed that at LSF,
> the people thought we will be using a lot of memory for sub-block
> memory, especially on architectures with 64k base page size.
> 
> So for now a good tradeoff between memory usage and efficiency was to
> use a 64k buffer as that is the maximum FSB we support.[1]
> 
> IIUC, you will be using this function also to zero out the extent and
> not just a FSB?

Right. Or more specifically, the FS can ask for the zeroing size. 
Typically it will be inode i_blocksize, but the FS can ask for a larger 
size to zero out to extent alignment boundaries.

> 
> I think we could resort to looping until we have a way to request
> arbitrary zero folios without having to allocate at it in
> iomap_dio_alloc_bio() for every IO.
> 

ok

> [1]https://urldefense.com/v3/__https://lore.kernel.org/linux-xfs/20240529134509.120826-8-kernel@pankajraghav.com/__;!!ACWV5N9M2RV99hQ!MTwVaC6oueHR_vgmDfOvgBX8bPdeTSRPcRcw5-CqtHnFEH-Ya1sUeZwaF-xrBF5XZ_8lJw5l-riq4t8Ij2hl9yU$  

Thanks,
John