Received-SPF: pass (google.com: domain of linux-kernel+bounces-209386-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <4c6e092d-5580-42c8-9932-b42995e914be@oracle.com>
Date: Tue, 11 Jun 2024 08:38:34 +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>, david@fromorbit.com,
        djwong@kernel.org, chandan.babu@oracle.com, brauner@kernel.org,
        akpm@linux-foundation.org, willy@infradead.org
Cc: 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>
Content-Language: en-US
From: John Garry <john.g.garry@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <20240607145902.1137853-8-kernel@pankajraghav.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?akNXSGVRZEZTRkRPS3RkR0xUTCs3ZmQ1aHdLM1A5ZjRpc3hIT0c1TVBLRXk4?=
 =?utf-8?B?YlRXNU02QnZUWERpVEJSb1g5Qjk3SGg2Wk5jLzhIT0VBdE9ueENsQm1HWEdn?=
 =?utf-8?B?MSsvM2ErZW5jMklBRXRNZUVPRGgxbkJSeUFqeDQ5dzZkWUlFYkRsbk0zc3JC?=
 =?utf-8?B?cWZRTHZZRGpxWm01Lzl5b0hzajE5VjNwMDA4RXBLYnNWcTdYWmZxS3hOYzNj?=
 =?utf-8?B?dVJIZVBiUUFDMHlOUEcrYUIwOXpyVjlUcWQ3YkNSN2tjbi9CY0p2ZERTUzR5?=
 =?utf-8?B?bjRZdjVwWWcvOC9CRGh5dDBlU2lFaDVVYWhKMWE5R3ZyUXdEaUM3QnVrL3V2?=
 =?utf-8?B?UjNZd3JIZ0JpUVhCUEFRQkxxYXVrZkZBZnhMeklrQy9DTjdib2dYWkJLL3Zx?=
 =?utf-8?B?N0wyK0tZazVmQlo5TUpFWHlEQjJPekJUNCtId1RDc2gzQnVkaEJRYjhpM3kv?=
 =?utf-8?B?dnVHYjBtKzJpZnJKOGpxbTNVM2VUY0hGN09Dd2FhL3AwNVFrVDd1aWFJTmZm?=
 =?utf-8?B?aVlmL0dWOGlhbGpmRWRlejJaM1pGOU50U2VuMG52bDZBWDZSQ1ZkSmtCejhG?=
 =?utf-8?B?bmRNa3o1RlBzUmtXUkd0dWhaSVMvV3pRa3cwcjhoelBaVGkyRUNCUVJNcVVh?=
 =?utf-8?B?d1p5RE1iQXRvTXkrd2ppeGVVTlViWmdxdUlSckNyR0c5djBoN0hsN3JDbkdm?=
 =?utf-8?B?Z2dkWDBYeitCc09IbndkRVRKWTgwZmJQVXdOVFg1REx0cFQxdEhSWG1tTnJr?=
 =?utf-8?B?S2pmS2tiMXpmUDREMDVuSzU5SUZWYTZVakdibHgwTTZWdWFnRG4wT0dyM1hH?=
 =?utf-8?B?MUoxSml3SlRpUUpWUkZkM3lrUkhXWDBqUmg4dXE3R1lsbkQvbzBKOWFxN2pH?=
 =?utf-8?B?N2hBMWhRZS9RcGRacUcvd2lLVzlMbTZESFRMc21FSDV4ZGxmdU5CRXpDWGYw?=
 =?utf-8?B?TlYzOE5RS1krRWh2VnpYR2ZONzRvazJmVS9rZHBldWFncThEc0RvVisvVSts?=
 =?utf-8?B?YytlTmVWb3FkZWdocDlleGJ0N0w0dXdUaFUzN01pcThNMHdMTjZUVEVMbzdC?=
 =?utf-8?B?dVJCdml4c0pUUDY1bDlxWDNpd2VYY2JMV2RnQ0ltS3RFRnpWaFRUNk53MnBr?=
 =?utf-8?B?MFg2OGdaQ1oxS0pMZWpINHI4VHdBeFZ2ZkVWV29KVlV6Rm1mdlpiM2VMbk5R?=
 =?utf-8?B?dTF1eVJScEQ0cnp2MHNOZGdpTWlSYXl3Rmg1Ri8rclBacFZWQmhFK2Q5YmVX?=
 =?utf-8?B?dktiTmx6bTdRRzdDbUFWOUF2L2lTY0M5MlZ5WmdJZGxUVjVzMCtDaTF2STVJ?=
 =?utf-8?B?TTExSmg3TzJFVExVbDFKYkQzSFhUK0ZCa3pZaUJtRGlDU24zWTNudUY5R1JT?=
 =?utf-8?B?UDhsZWhPeHE1b3FXa080NjdaSUNBc0dQcEdYSzNPc2pWU3VDZ3FRM3dIbDlD?=
 =?utf-8?B?SFVUSmJoQVJEN3dlK0hhRG5jSm9HaGo2R2xyanNNMWVvQi9jU2h3dENmK1Ju?=
 =?utf-8?B?YlhyU1FrK0FzSDRwSXI3Sm5NS2RMVFIrQjZvUFNRb1VLK3lUaU9WZlZCT1NX?=
 =?utf-8?B?WmMvTUpkYy9lUzRVSlc0NGU4cHJrK0RMN0d2dStlU3dFbnc0a3lhVzkvdkZn?=
 =?utf-8?B?clR5ZWZzRithdUlmcFVTTzkyaEtlK1piVy9FUlVaWGdPTXhiNUtnL3RHc2th?=
 =?utf-8?B?RUtQdXFuYVhVNG1JT1JwY2xqOW1SYW5CRzV2YWc4djJQdDd1YUFURTQydE52?=
 =?utf-8?B?MnBHbjRwVlk1MENlOTQ2ZGlaemw4QzUxaTExT3dLMEhBT0F4T2xLeS9CTnZC?=
 =?utf-8?B?cjlqSU9XNDNTZVN3K3QrSnhiWVRCUmFhc1lpRW0zOUNRNDhaWTZnaXQxZFk0?=
 =?utf-8?B?VzAzbTJBYlJSUDlLR1pMNUFjWVhseWtXS2c4cXJYZDN3STY4RDluVXRUMUVQ?=
 =?utf-8?B?L25lM2dLc2RIdmI4VkdHMDNwUjZGekRLRDlqVGFybUhBb2JTSHFlbTRvcjY0?=
 =?utf-8?B?cDBLMWczRlFHVlNlb3RBL2h6NWxDbllxRThMZW9mM21ReGdhSlZMN1JhckJx?=
 =?utf-8?B?elg0UmFEVDcvbG1VbFdYclhiVitWNFJPQnVmNHo5NXZwdWFwOTFRM2x0bWph?=
 =?utf-8?B?S2o0aVM0aXhzNDR6WDY0N3NuTTNmWFAyL3VjQzZCOExVTDlkMlRHKzJPekZw?=
 =?utf-8?B?bkE9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	F23HdRsyyMB9vBUafCJhBRMipqile18VsetuReBMZxOX+kYThGaAt4v9lwSx8PrB5mgbYqLDQgPJDIUUjW7jeWh/TB26iW7fHoCbBtecSu6Kl39K75W0HWKRXu6N0z1vyRNH+/o+ofgy/E8JS0XJLsBsdR0a1fc51/nq8UZYLrpae1JBKZc8YJUbdrydkYA5RKQT2gGVttRD6lvWiReeRSqkOZi1803zsqbMLFZmZjXPYIR2bRMtnEcOY/u0KhFW7aPj4fuxChk427mUHSmV8OxH67mWLBzNZyFphbETh5+O9qYAg/JK0Hm2U9KOhLuV2rEb6Y1FipmlvkdrspWZDSQGiLLArMk5eXS2qrIHXO3b4KwRSpVg/wsvrUBvr2vh0S7lcDrKWQQKwe90OrV+zmz7K/XdVX+xTkx8DPY4zN5KO3APb8PQgvt/gcW8hiKVmOYUEShOAVAICXTvYMlj7cUp1zJE70fHs6y92fzHYtiYaLYnAN0Eb3mq7pYh7trIzX3TE3U8r3nggxh3+YmkPevTBc8vqpuJF78VPAMrRrPzlzd9kRaWQ/4Bo3wDwz9PLAJyckEzdOAB29xyY6mTWt7DQkg8cG9+KllseWyizsM=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: be15e079-b49f-472c-81b4-08dc89e98395
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB4313.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Jun 2024 07:38:41.0100
 (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: S7oRZL/L4aPPi1DsdLOClsqLGAAMtJqgzQWoGAC51YealfNH0R/2RAlV3Mzqcy59fLbuCT9tYGcPLnUuBiTJBA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR10MB6724
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_03,2024-06-11_01,2024-05-17_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 adultscore=0
 phishscore=0 suspectscore=0 malwarescore=0 mlxscore=0 spamscore=0
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2405010000 definitions=main-2406110057
X-Proofpoint-GUID: 9vxlTjxvw2_YPl273lL4CRZtp7o-e0_Y
X-Proofpoint-ORIG-GUID: 9vxlTjxvw2_YPl273lL4CRZtp7o-e0_Y

On 07/06/2024 15:58, Pankaj Raghav (Samsung) wrote:
> From: Pankaj Raghav <p.raghav@samsung.com>
> 
> iomap_dio_zero() will pad a fs block with zeroes if the direct IO size
> < fs block size. iomap_dio_zero() has an implicit assumption that fs block
> size < page_size. This is true for most filesystems at the moment.
> 
> If the block size > page size, this will send the contents of the page
> next to zero page(as len > PAGE_SIZE) to the underlying block device,
> causing FS corruption.
> 
> iomap is a generic infrastructure and it should not make any assumptions
> about the fs block size and the page size of the system.
> 
> Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> ---
>   fs/internal.h          |  5 +++++
>   fs/iomap/buffered-io.c |  6 ++++++
>   fs/iomap/direct-io.c   | 26 ++++++++++++++++++++++++--
>   3 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/internal.h b/fs/internal.h
> index 84f371193f74..30217f0ff4c6 100644
> --- a/fs/internal.h
> +++ b/fs/internal.h
> @@ -35,6 +35,11 @@ static inline void bdev_cache_init(void)
>   int __block_write_begin_int(struct folio *folio, loff_t pos, unsigned len,
>   		get_block_t *get_block, const struct iomap *iomap);
>   
> +/*
> + * iomap/direct-io.c
> + */
> +int iomap_dio_init(void);
> +
>   /*
>    * char_dev.c
>    */
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index 49938419fcc7..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.

> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> index f3b43d223a46..b95600b254a3 100644
> --- a/fs/iomap/direct-io.c
> +++ b/fs/iomap/direct-io.c
> @@ -27,6 +27,13 @@
>   #define IOMAP_DIO_WRITE		(1U << 30)
>   #define IOMAP_DIO_DIRTY		(1U << 31)
>   
> +/*
> + * Used for sub block zeroing in iomap_dio_zero()
> + */
> +#define ZERO_FSB_SIZE (65536)
> +#define ZERO_FSB_ORDER (get_order(ZERO_FSB_SIZE))
> +static struct page *zero_fs_block;
> +
>   struct iomap_dio {
>   	struct kiocb		*iocb;
>   	const struct iomap_dio_ops *dops;
> @@ -52,6 +59,16 @@ struct iomap_dio {
>   	};
>   };
>   
> +int iomap_dio_init(void)
> +{
> +	zero_fs_block = alloc_pages(GFP_KERNEL | __GFP_ZERO, ZERO_FSB_ORDER);
> +
> +	if (!zero_fs_block)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
>   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 in 
https://lore.kernel.org/linux-xfs/20240429174746.2132161-1-john.g.garry@oracle.com/T/#m5354e2b2531a5552a8b8acd4a95342ed4d7500f2, 
we would like to support an arbitrary size. Maybe I will need to loop 
for zeroing sizes > 64K.

> +
>   	bio = iomap_dio_alloc_bio(iter, dio, 1, REQ_OP_WRITE | REQ_SYNC | REQ_IDLE);
>   	fscrypt_set_bio_crypt_ctx(bio, inode, pos >> inode->i_blkbits,
>   				  GFP_KERNEL);
> +
>   	bio->bi_iter.bi_sector = iomap_sector(&iter->iomap, pos);
>   	bio->bi_private = dio;
>   	bio->bi_end_io = iomap_dio_bio_end_io;
>   
> -	__bio_add_page(bio, page, len, 0);
> +	__bio_add_page(bio, zero_fs_block, len, 0);
>   	iomap_dio_submit_bio(iter, dio, bio, pos);
>   }
>