From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 1BAC11D54E2; Tue, 8 Jul 2025 20:20:51 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1752006052; cv=none; b=dEPLau6gg5JBwuabax/pHvDyJMiiWCsRdkAkFnYeGj4h+V7MKGVtrUtFfyKJ0E/0kgcOCJdvDCHNblJnOnGgk+9ruZguYG/TVN0bScVecxJZooQPdIWV+jqJF9XJmUuN3WjnRb1FHGn4sjSd25edXqxM/e+6cV+wxmos4yynScU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1752006052; c=relaxed/simple; bh=+yxZ08dKrAbdFZ6sAL+9jMwOYAMn3pq6/TMoNHiAVqM=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=POqngLEtnY7UOEA1+VhFCNGe5YPKhl9DFMSdzSwhChJOxQopnmP7kwSOjfvFZHJ35dn/m7YMKRBX4K3+S5PZayMHntqKPQ7Dqxf/gmhWoLhnBUp3EMwOcl3iXvrC7zUZ0mtFvXwAYTI/PZpFsvHdIsVJQcfNkCwOuugM3Gc3oX0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=hPMEhSkU; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="hPMEhSkU" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 58E7EC4CEED; Tue, 8 Jul 2025 20:20:51 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1752006051; bh=+yxZ08dKrAbdFZ6sAL+9jMwOYAMn3pq6/TMoNHiAVqM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=hPMEhSkUX/LsvtZLeds9iEsFR445eshxWZXvoRnSLujgLEPzAcfHTEY1hKzvOZwLi CO6xBgdyPLvwJKOyvRvQbfayjvwz//qmWRwCqXX9eJfnurVWX9yJjB+FGnGJqw9Bdo YvTa4ZUzOiq4x/0+MnhVWgvty8CXom2KMAk1lU1DdgCV3QPlLlzl63cgXPodHeUmuR c/DyafLAONZHqILRSCVnIMh6DkLySD8sZvd+yvzKsb2YGHyNZ8+dzvy79GVsPU2CIa QSxZLDXRKYoNFJq9mC7u92Mfs4HyGdJgpKNx3bbNB6DnNtxSNNOGKynr+rejYv2LwT nVQjowPhBvE+A== Date: Tue, 8 Jul 2025 13:20:50 -0700 From: "Darrick J. Wong" To: Jan Kara Cc: Qu Wenruo , Dave Chinner , Qu Wenruo , linux-btrfs@vger.kernel.org, linux-fsdevel@vger.kernel.org, viro@zeniv.linux.org.uk, brauner@kernel.org, linux-ext4@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, ntfs3@lists.linux.dev, linux-xfs@vger.kernel.org Subject: Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev() Message-ID: <20250708202050.GG2672049@frogsfrogsfrogs> References: <20250708004532.GA2672018@frogsfrogsfrogs> <2dm6bsup7vxwl4vwmllkvt5erncirr272bov4ehd5gix7n2vnw@bkagb26tjtj5> Precedence: bulk X-Mailing-List: linux-ext4@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <2dm6bsup7vxwl4vwmllkvt5erncirr272bov4ehd5gix7n2vnw@bkagb26tjtj5> On Tue, Jul 08, 2025 at 12:20:00PM +0200, Jan Kara wrote: > On Mon 07-07-25 17:45:32, Darrick J. Wong wrote: > > On Tue, Jul 08, 2025 at 08:52:47AM +0930, Qu Wenruo wrote: > > > 在 2025/7/8 08:32, Dave Chinner 写道: > > > > On Fri, Jul 04, 2025 at 10:12:29AM +0930, Qu Wenruo wrote: > > > > > Currently all the filesystems implementing the > > > > > super_opearations::shutdown() callback can not afford losing a device. > > > > > > > > > > Thus fs_bdev_mark_dead() will just call the shutdown() callback for the > > > > > involved filesystem. > > > > > > > > > > But it will no longer be the case, with multi-device filesystems like > > > > > btrfs and bcachefs the filesystem can handle certain device loss without > > > > > shutting down the whole filesystem. > > > > > > > > > > To allow those multi-device filesystems to be integrated to use > > > > > fs_holder_ops: > > > > > > > > > > - Replace super_opearation::shutdown() with > > > > > super_opearations::remove_bdev() > > > > > To better describe when the callback is called. > > > > > > > > This conflates cause with action. > > > > > > > > The shutdown callout is an action that the filesystem must execute, > > > > whilst "remove bdev" is a cause notification that might require an > > > > action to be take. > > > > > > > > Yes, the cause could be someone doing hot-unplug of the block > > > > device, but it could also be something going wrong in software > > > > layers below the filesystem. e.g. dm-thinp having an unrecoverable > > > > corruption or ENOSPC errors. > > > > > > > > We already have a "cause" notification: blk_holder_ops->mark_dead(). > > > > > > > > The generic fs action that is taken by this notification is > > > > fs_bdev_mark_dead(). That action is to invalidate caches and shut > > > > down the filesystem. > > > > > > > > btrfs needs to do something different to a blk_holder_ops->mark_dead > > > > notification. i.e. it needs an action that is different to > > > > fs_bdev_mark_dead(). > > > > > > > > Indeed, this is how bcachefs already handles "single device > > > > died" events for multi-device filesystems - see > > > > bch2_fs_bdev_mark_dead(). > > > > > > I do not think it's the correct way to go, especially when there is already > > > fs_holder_ops. > > > > > > We're always going towards a more generic solution, other than letting the > > > individual fs to do the same thing slightly differently. > > > > On second thought -- it's weird that you'd flush the filesystem and > > shrink the inode/dentry caches in a "your device went away" handler. > > Fancy filesystems like bcachefs and btrfs would likely just shift IO to > > a different bdev, right? And there's no good reason to run shrinkers on > > either of those fses, right? > > I agree it is awkward and bcachefs avoids these in case of removal it can > handle gracefully AFAICS. > > > > Yes, the naming is not perfect and mixing cause and action, but the end > > > result is still a more generic and less duplicated code base. > > > > I think dchinner makes a good point that if your filesystem can do > > something clever on device removal, it should provide its own block > > device holder ops instead of using fs_holder_ops. I don't understand > > why you need a "generic" solution for btrfs when it's not going to do > > what the others do anyway. > > Well, I'd also say just go for own fs_holder_ops if it was not for the > awkward "get super from bdev" step. As Christian wrote we've encapsulated > that in fs/super.c and bdev_super_lock() in particular but the calling > conventions for the fs_holder_ops are not very nice (holding > bdev_holder_lock, need to release it before grabbing practically anything > else) so I'd have much greater peace of mind if this didn't spread too > much. Once you call bdev_super_lock() and hold on to sb with s_umount held, > things are much more conventional for the fs land so I'd like if this > step happened before any fs hook got called. So I prefer something like > Qu's proposal of separate sb op for device removal over exporting > bdev_super_lock(). Like: > > static void fs_bdev_mark_dead(struct block_device *bdev, bool surprise) > { > struct super_block *sb; > > sb = bdev_super_lock(bdev, false); > if (!sb) > return; > > if (sb->s_op->remove_bdev) { > sb->s_op->remove_bdev(sb, bdev, surprise); > return; > } It feels odd but I could live with this, particularly since that's the direction that brauner is laying down. :) Do we still need to super_unlock_shared here? --D > > if (!surprise) > sync_filesystem(sb); > shrink_dcache_sb(sb); > evict_inodes(sb); > if (sb->s_op->shutdown) > sb->s_op->shutdown(sb); > > super_unlock_shared(sb); > } > > > As an aside: > > 'twould be nice if we could lift the *FS_IOC_SHUTDOWN dispatch out of > > everyone's ioctl functions into the VFS, and then move the "I am dead" > > state into super_block so that you could actually shut down any > > filesystem, not just the seven that currently implement it. > > Yes, I should find time to revive that patch series... It was not *that* > hard to do. > > Honza > -- > Jan Kara > SUSE Labs, CR >