From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id C1282C4345F for ; Mon, 29 Apr 2024 14:59:50 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 57B106B0099; Mon, 29 Apr 2024 10:59:50 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 503846B009E; Mon, 29 Apr 2024 10:59:50 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 37C626B00A0; Mon, 29 Apr 2024 10:59:50 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 176906B0099 for ; Mon, 29 Apr 2024 10:59:50 -0400 (EDT) Received: from smtpin06.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 96109120324 for ; Mon, 29 Apr 2024 14:59:49 +0000 (UTC) X-FDA: 82062878898.06.A01EFE5 Received: from mail-pg1-f176.google.com (mail-pg1-f176.google.com [209.85.215.176]) by imf18.hostedemail.com (Postfix) with ESMTP id DF2FC1C0017 for ; Mon, 29 Apr 2024 14:59:46 +0000 (UTC) Authentication-Results: imf18.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=JlBWwWYZ; spf=pass (imf18.hostedemail.com: domain of ritesh.list@gmail.com designates 209.85.215.176 as permitted sender) smtp.mailfrom=ritesh.list@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1714402786; a=rsa-sha256; cv=none; b=m7niBeM/EsZFFGtmSHmfNPIiVQ8SUNKk5LFMAA0NFXC+Eh1qBK5H+IWBuXgF71ocFsBZXj Sj9GwqKv3LBpEg75g8XzAfX3SHU0NQGYFPoWVy3KbBib1D3HivAYago+32ittuCnxgQpeP sRYKl4dwu3ZipRaPksHn3LI6G6ifuwg= ARC-Authentication-Results: i=1; imf18.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=JlBWwWYZ; spf=pass (imf18.hostedemail.com: domain of ritesh.list@gmail.com designates 209.85.215.176 as permitted sender) smtp.mailfrom=ritesh.list@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1714402786; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:content-type: content-transfer-encoding:in-reply-to:in-reply-to:references:dkim-signature; bh=D2i2h/fU62HBi0Yob2v06SoPeokjr50XNhm9YQdF1O4=; b=3t/C6VM+V8idRKL5o8xaiFNSQ9ETE/SXAejCmzSiOO3aUyzoZRI/usEVGNqAA0JFN+ptix 9lIjqEl73vFRjVfRM/lk4n6Vs9jTFk9Dc7DGYD1/ywsW/qOVM2yULlbFcoceXiqSHFGlhf 8kZm/KWOFMWyuJEfA4EctvprtoLecWQ= Received: by mail-pg1-f176.google.com with SMTP id 41be03b00d2f7-53fbf2c42bfso3250710a12.3 for ; Mon, 29 Apr 2024 07:59:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1714402786; x=1715007586; darn=kvack.org; h=in-reply-to:subject:cc:to:from:message-id:date:from:to:cc:subject :date:message-id:reply-to; bh=D2i2h/fU62HBi0Yob2v06SoPeokjr50XNhm9YQdF1O4=; b=JlBWwWYZ4k7CWRkJwyyvbYP4t7cym2+AD6NZPofRX/IlWkj3TTZxXMpXT+aJWAT6XE /7Vdoku96UxP40bGpdhMZWRkyPFWdGtpDNoEQbxcKsH8Mba283Lpr6Nk3cTT13Tx3n8x G2ryeLAtWl1NieKKuSCmx3eGQrNyWYkPoMYWyiNDyrYtMwNPb9RIzfBNwYRkSUad39Ih /iFV6QQDcPSB10Ej5K19AsOXJa8m9jmovnEfO0deBEyGvZtGqGnycM48LRuAeJ6o4idz mDldV6dxC1yJwErD+iJHbPq/hh6KSrlTxpmZPRTxWej1WksrFhevk6VLsut37rSyZsjV zCag== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714402786; x=1715007586; h=in-reply-to:subject:cc:to:from:message-id:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=D2i2h/fU62HBi0Yob2v06SoPeokjr50XNhm9YQdF1O4=; b=I+qNnNkb+c2PWasAkN3duwcuk84Dsa10FAo4O+ZQJCNgPxg6OAXRNn5ibGcJljUzvi Dbuhd6q4nygxo+nrSFeGIyTIVDKb2v8lXzFysRyx8CzRgmvX62JEud3BBTUsPdv0tvdM caXyU20RhSl0hKuHsDVDJre4EDHtIL5zqqXkckPQOF5PS3wR40wCaRjrWl2eyDCZQGQD eSmtf5Pj3mm0yk4mw4MDVQvrdh/HpfbcbDXoAsJNXrIz02uxOBwlWEETHv561xr0/6/D kna3tNm75DmPrDOqArSe+tOSrvkTLacEkDvO65UqN9pXAJr1sQIGYrlxD2EODLEULuD9 AFqg== X-Forwarded-Encrypted: i=1; AJvYcCVd4YmM8iCrZfLH0kXkoC14vsuOvf8zL4Itn+3jO4pt7PJk9JkQdO2Bdp8+GGmAB7ukoyCbnpmwDy8d7c/9LkmtFvg= X-Gm-Message-State: AOJu0YxG+absiQPPsIDVim8YoIUtQUVkG2UywdEqMF6EsOMGB4EMZH3l bfxM5Bee70ksDt18UKXuXM0mJNoS5x+WsfoiPrTkGT3doIK5nWaE X-Google-Smtp-Source: AGHT+IFrQ4pzYZvOZbN3sE0v0c88khjAywVfp5oa41UWydT1M8w695R8CfnRvziWkYDWRWB95h5gNg== X-Received: by 2002:a05:6a20:748b:b0:1a7:7358:f111 with SMTP id p11-20020a056a20748b00b001a77358f111mr14510571pzd.31.1714402785678; Mon, 29 Apr 2024 07:59:45 -0700 (PDT) Received: from dw-tp ([171.76.84.250]) by smtp.gmail.com with ESMTPSA id w12-20020a170902a70c00b001e45c0d6be6sm20463521plq.246.2024.04.29.07.59.40 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 29 Apr 2024 07:59:45 -0700 (PDT) Date: Mon, 29 Apr 2024 20:29:38 +0530 Message-Id: <87o79sxlid.fsf@gmail.com> From: Ritesh Harjani (IBM) To: Zhang Yi , linux-ext4@vger.kernel.org Cc: linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, tytso@mit.edu, adilger.kernel@dilger.ca, jack@suse.cz, hch@infradead.org, djwong@kernel.org, david@fromorbit.com, willy@infradead.org, zokeefe@google.com, yi.zhang@huawei.com, chengzhihao1@huawei.com, yukuai3@huawei.com, wangkefeng.wang@huawei.com Subject: Re: [PATCH v4 02/34] ext4: check the extent status again before inserting delalloc block In-Reply-To: <3243c67d-e783-4ec5-998f-0b6170f36e35@huaweicloud.com> X-Stat-Signature: nr5bcca9dz71h1grkm11qceftaiwy48f X-Rspam-User: X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: DF2FC1C0017 X-HE-Tag: 1714402786-715467 X-HE-Meta: U2FsdGVkX18+dX4EetPxAVfCZfsKVpkF37A8Z2epKLDiyXWFIfWHKa4cNkWD0l1YTfJHStiqbomATOupovxlyIoMm8TjMNigkobaqeT04s2y02a6n/UwXR9flpFUCte6rXDPQ8QeMS9/tY24jRpOfPZfv0tLSJU88wXoWQG7un2nWJi9Q1gb8/YEztesO6faazJ73Yab+L+XRV4D5TsYTcTX6+2biO4d2ci5WkS72pTGTwjU5YQ3lsA09WErTzPa4lCn9/F585BG3qr916U5JhyUDj9+xoU1b5KqBilk266gWC4qmAJNbBNaEWqBl1DF0OeyA7hsVztNqJC2JM2ROnW4OS9DD9JkjPEUMpVYWvEmM391MvJ6M9+WoIgx5ij6Y7+uH2aXZOf43hMwDh0mO/ZL4wxm658ssVvS/+UzmGRc2u6lCFQqp3eS1nrlM3pcnBXjJqIqT7x2IDkzi1afScIG3FE8kbV4RX42CF0vu+rCYbZ4WWBV7BPLfuX54CZI2C6HX1wp02vXSXVehmJ+feWu5ZJNyW/+jrYpWnF7v/Kgs7jAU4xyNFRGCaJ4DUpZTwfzQ4qA6KuLWDZ+Hc9bxOi5IUpvP61WFK1EBNo86ljEnn2yGrA/vY5rQuXHbxIe5+FrRPzC30FJirvBqB5/U+5vHeh6j6R1zIU9YvdiWv8xXsq677tzjX93ITkAwbcrZ5sXa3Ig/+3FcKxMSRY/18VkyGbLUxQqECIAARxhz86OxT5+fm9HHq1u83gLzeaaHi0e6NtmCd3vJpECZGJTcOsVdEKxo16+yCB6K92x4agIsqjxezFuYKD9b+kej/JdgJEYLqcS0YqpCP3h4qqJtS6Lsy97v7ypr7dvCdvapu9GAuumDDM5tWSXVGvMz1EyTa7R1UHHwuS9UjYhzIM508/RbCfe9aSZ+mc6DadmuFECw1FUiCBYoIW5ZQHDh3Q37UMIkTkbCwZXrr1Xwlp jB/zuJ9t /lDInudlyGJD9V14d3TTwRHYcb8sIpfoAIVjCYAdDZxWBEXkNPNSuOa9lUA3AKD3OWiciICtpk4LVkrQZmSwb9BTMmWEZl1MPxpDcZzzP7i9HZx8+YAsnHv36U9ufRovq4dBjEKUES31DYSUhednAvArDb2QMgaL6I/w9dnINwc9MyuPgnXIefk/24RwUAVUgIKxmyO0A9iMojL6+dUCUmB71nMI5pV9MJ1ykCIZjEHzAx9KCu/Q/bMwXYjWmKuupwfcRmvcRYyDHfPM4mRAHLktZ+IeinI6CPExHWrXRQznt3lFHPP7D7XggiUFWujtstJMAsxBxyrM7JngruaOuDPfuRL6hKZBMxoAZY0E2BSLLdKw+rsk5z2+3gcJ+87pmburRQ0Eg7ODkjbwgadkPF4LkgwNyeChCHno9lYEnC6mOZb8oeHgMZUlK0kdB8PAzKa5cpBqfN3Aq0NP4AG2ty2S7U6CRD8IwVSeRx6DuX6USSKE2JNWw0yyhJcEjytAU/jpyS8HkPHbUSJuX5JOgL/d4jppEpsV9+hexGhEtOzJVWrXgN8dzDGOxW13RtIubExjq X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: Zhang Yi writes: > On 2024/4/27 0:39, Ritesh Harjani (IBM) wrote: >> Zhang Yi writes: >> >>> On 2024/4/26 20:57, Ritesh Harjani (IBM) wrote: >>>> Ritesh Harjani (IBM) writes: >>>> >>>>> Zhang Yi writes: >>>>> >>>>>> From: Zhang Yi >>>>>> >>>>>> Now we lookup extent status entry without holding the i_data_sem before >>>>>> inserting delalloc block, it works fine in buffered write path and >>>>>> because it holds i_rwsem and folio lock, and the mmap path holds folio >>>>>> lock, so the found extent locklessly couldn't be modified concurrently. >>>>>> But it could be raced by fallocate since it allocate block whitout >>>>>> holding i_rwsem and folio lock. >>>>>> >>>>>> ext4_page_mkwrite() ext4_fallocate() >>>>>> block_page_mkwrite() >>>>>> ext4_da_map_blocks() >>>>>> //find hole in extent status tree >>>>>> ext4_alloc_file_blocks() >>>>>> ext4_map_blocks() >>>>>> //allocate block and unwritten extent >>>>>> ext4_insert_delayed_block() >>>>>> ext4_da_reserve_space() >>>>>> //reserve one more block >>>>>> ext4_es_insert_delayed_block() >>>>>> //drop unwritten extent and add delayed extent by mistake >>>>>> >>>>>> Then, the delalloc extent is wrong until writeback, the one more >>>>>> reserved block can't be release any more and trigger below warning: >>>>>> >>>>>> EXT4-fs (pmem2): Inode 13 (00000000bbbd4d23): i_reserved_data_blocks(1) not cleared! >>>>>> >>>>>> Hold i_data_sem in write mode directly can fix the problem, but it's >>>>>> expansive, we should keep the lockless check and check the extent again >>>>>> once we need to add an new delalloc block. >>>>> >>>>> Hi Zhang, >>>>> >>>>> It's a nice finding. I was wondering if this was caught in any of the >>>>> xfstests? >>>>> >>> >>> Hi, Ritesh >>> >>> I caught this issue when I tested my iomap series in generic/344 and >>> generic/346. It's easy to reproduce because the iomap's buffered write path >>> doesn't hold folio lock while inserting delalloc blocks, so it could be raced >>> by the mmap page fault path. But the buffer_head's buffered write path can't >>> trigger this problem, >> >> ya right! That's the difference between how ->map_blocks() is called >> between buffer_head v/s iomap path. In iomap the ->map_blocks() call >> happens first to map a large extent and then it iterate over all the >> locked folios covering the mapped extent for doing writes. >> Whereas in buffer_head while iterating, we first instantiate/lock the >> folio and then call ->map_blocks() to map an extent for the given folio. >> >> ... So this opens up this window for a race between iomap buffered write >> path v/s page mkwrite path for inserting delalloc blocks entries. >> >>> the race between buffered write path and fallocate path >>> was discovered while I was analyzing the code, so I'm not sure if it could >>> be caught by xfstests now, at least I haven't noticed this problem so far. >>> >> >> Did you mean the race between page fault path and fallocate path here? >> Because buffered write path and fallocate path should not have any race >> since both takes the inode_lock. I guess you meant page fault path and >> fallocate path for which you wrote this patch too :) > > Yep. > >> >> I am surprised, why we cannot see the this race between page mkwrite and >> fallocate in fstests for inserting da entries to extent status cache. >> Because the race you identified looks like a legitimate race and is >> mostly happening since ext4_da_map_blocks() was not doing the right >> thing. >> ... looking at the src/holetest, it doesn't really excercise this path. >> So maybe we can writing such fstest to trigger this race. >> > > I guess the stress tests and smoke tests in fstests have caught it, > e.g. generic/476. Since there is only one error message in ext4_destroy_inode() > when the race issue happened, we can't detect it unless we go and check the logs > manually. Hi Zhang, I wasn't able to reproduce the any error messages with generic/476. > > I suppose we need to add more warnings, something like this, how does it sound? > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index c8b691e605f1..4b6fd9b63b12 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -1255,6 +1255,8 @@ static void ext4_percpu_param_destroy(struct ext4_sb_info *sbi) > percpu_counter_destroy(&sbi->s_freeclusters_counter); > percpu_counter_destroy(&sbi->s_freeinodes_counter); > percpu_counter_destroy(&sbi->s_dirs_counter); > + WARN_ON_ONCE(!ext4_forced_shutdown(sbi->s_sb) && > + percpu_counter_sum(&sbi->s_dirtyclusters_counter)); > percpu_counter_destroy(&sbi->s_dirtyclusters_counter); > percpu_counter_destroy(&sbi->s_sra_exceeded_retry_limit); > percpu_free_rwsem(&sbi->s_writepages_rwsem); > @@ -1476,7 +1478,8 @@ static void ext4_destroy_inode(struct inode *inode) > dump_stack(); > } > > - if (EXT4_I(inode)->i_reserved_data_blocks) > + if (!ext4_forced_shutdown(inode->i_sb) && > + WARN_ON_ONCE(EXT4_I(inode)->i_reserved_data_blocks)) > ext4_msg(inode->i_sb, KERN_ERR, > "Inode %lu (%p): i_reserved_data_blocks (%u) not cleared!", > inode->i_ino, EXT4_I(inode), > I also ran ext4 -g auto and I couldn't reproduce anything with above patch. Please note that I didn't use this patch series for testing. I was running xfstests on upstream kernel with above diff (because that's what the idea was that the problem even exists in upstream kernel and are we able to observe the race with page mkwrite and fallocate path) -ritesh > > Thanks, > Yi. > >> >>>>> I have reworded some of the commit message, feel free to use it if you >>>>> think this version is better. The use of which path uses which locks was >>>>> a bit confusing in the original commit message. >>>>> >>> >>> Thanks for the message improvement, it looks more clear then mine, I will >>> use it. >>> >> >> Glad, it was helpful. >> >> -ritesh >>