From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pj1-f44.google.com (mail-pj1-f44.google.com [209.85.216.44]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6135A125A9 for ; Wed, 15 Oct 2025 00:02:18 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.216.44 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1760486542; cv=none; b=faALfUpwADXyufvDwkJg1yzS7AV6b5z8dmNVI47HbuOx9fqoNHlv6XS5huAeZP57iK0RkoNFXSzwDEwVdo9pGbeMPLK3X9vyFPj7j1bTj6jh0qMctk+rqSX96Ljwi/5Q+0qbMEr0U10N8XMENoxX/5cj8Qv9XRwVMpyJJ/9vjVQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1760486542; c=relaxed/simple; bh=GJVi7Uls4ieWZ3bCIzyejkrCVwWN+vWqRyC0z9mQwkI=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=W/FgqFsjDxT/Ori73ltRYXTgAbnXqQ3Hb8kqDsWpTxOv5menoSLd4z3zdITlNmFoxSaPnc9fp7uVmQD4iVPXipaUs9juVK/IjEdsOM7Tr8MBgC7Teflz0TeJmfoo9jNSXvj68cLYBAsT4M9n+8zo8rd/hw+j1nQZX/xTLivPyZY= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=fromorbit.com; spf=pass smtp.mailfrom=fromorbit.com; dkim=pass (2048-bit key) header.d=fromorbit-com.20230601.gappssmtp.com header.i=@fromorbit-com.20230601.gappssmtp.com header.b=eNvsGT62; arc=none smtp.client-ip=209.85.216.44 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=fromorbit.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=fromorbit.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=fromorbit-com.20230601.gappssmtp.com header.i=@fromorbit-com.20230601.gappssmtp.com header.b="eNvsGT62" Received: by mail-pj1-f44.google.com with SMTP id 98e67ed59e1d1-33b8a8aa73bso1794572a91.0 for ; Tue, 14 Oct 2025 17:02:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=fromorbit-com.20230601.gappssmtp.com; s=20230601; t=1760486538; x=1761091338; darn=vger.kernel.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=5lWnsuwkzpeApWk2ViOA5FQ8lFTBV0LeMgWGpahdtj0=; b=eNvsGT620FsuqNk3mQttW1M9Tz0dvDGYG3obDZzkYv76UULuvqq6pmbJt41Exq1ftT OFaJ2uVM7FVyVg/XqTniQzbXxRyiSGtWfLzLvnpgJ0+srLasKhRVGALb/ADwS9h1kgQC Y+wk4C3rPvUA+Fhgzj72jmz8koRmj8xyWnsLYx0bJFJo34+TM5vLziRE7748uIW6KHZh pCyiGYaLAgeICDO1ta5v/ivQz5jQbhjmJlSitGwuqfzqpY2ZtXuf8SJlPv8pzehu4pSq R9MU4LtWzZGPAajjEYeGZjgYLN9a1yKiExVYHPTj3FJ8ph06LZ6s7DOQEnuFLZv2ThzF p3sg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1760486538; x=1761091338; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=5lWnsuwkzpeApWk2ViOA5FQ8lFTBV0LeMgWGpahdtj0=; b=MRdiPZmJoqyAUHgmPZIwEbwHnEkM2vNGxiyL6yqamS5G0vbMf8T8dHCvd34m0xImbB +6wPS/RoDmJyKmk23Ev2MoudOcitaHh7qXUboDgggfzyfAoyyacNfznz/a6I0b+GklIN J2HBhm3tAh6CqY6a9zCKKZE9JZM4q+oPHBtNbrf6e5wUCP2lfW+uT26uxkPHcPQ7Q4HP P/Wg0OH19BbsnEkGCAx4COeeJ/+s3TNePDquFRY4fVQBFV8zi8Vn6zjxlA1X0tYr49e/ IuiaN4j7gojD6CvSP045QRz7k0AAQyg6NTMMB+Pt/MxdIwOWmiAXzqWLptnLXB/4MwPg 6jUg== X-Forwarded-Encrypted: i=1; AJvYcCV88wzizQssGkTtNrH7RlAhy6sPGfbgs2XIYwLoe8pSEou6jD59OgnUcbRi6lfPhG8255dH6S1pdqqK@vger.kernel.org X-Gm-Message-State: AOJu0YwE28n17336e30vP3t8CRj1Pe9+2ALqQltp4bjC3WJJw7HJFE4V uvhqpCdGEMSGCxCkJ+Uppe5VmJVqDgBb1f9HY6LFzfB3ivXV1JF8kWfkIM5LS1bAr/Q= X-Gm-Gg: ASbGncvV8KsWlHDhg4aj+ZajJw/HczVsCqAUDIU1ndu+UGApchDIDOM/x8e8xyOtuWd kq8B7rzJigU+JjeSY+7duEN95u2kjOjAbQTDvfq6dBSLoeJPFqH0bQ7Gr6U0LMQFqn6XIipUpRq if0zjOMzEDTaVpkyn3Sql+odj0wztuPz0kb+QN+V+ijhq3sB1QhyybxIcOoo6vsLfEXeY75FBdx S1Avl1OERNht+rfHxclwTrMAku9NPQjO4yIkcSv2zThRkRahIWHcmJaru2YgtI4ANa7i+XgfsWt djz5GCY7O1DTyVIGTm7VmScvemCizFXpGieNnaZXFSD3mgk7egAFM4+cUVPdOxClTCuUkhSv7Mv MLoI2Wei5TapC4NyZQ5ghctQ3UtbOHR1u7wWXpxnzVZGsXY2ia7a3hbD5ILMGdfusqXplsUt9ao NeMcvyMbimT9YRRENL X-Google-Smtp-Source: AGHT+IGoMUjacqS/V8o1WNbZPsHJJvFetdCeeZRacPCwQ/XYo2nNnoscuKOLgAnWu1qaNz9uyKQLiQ== X-Received: by 2002:a17:90b:4a84:b0:32e:a10b:ce48 with SMTP id 98e67ed59e1d1-33b51114f9emr38707009a91.12.1760486538088; Tue, 14 Oct 2025 17:02:18 -0700 (PDT) Received: from dread.disaster.area (pa49-180-91-142.pa.nsw.optusnet.com.au. [49.180.91.142]) by smtp.gmail.com with ESMTPSA id 98e67ed59e1d1-33b61b11ca9sm17310305a91.24.2025.10.14.17.02.17 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 14 Oct 2025 17:02:17 -0700 (PDT) Received: from dave by dread.disaster.area with local (Exim 4.98.2) (envelope-from ) id 1v8oy6-0000000EvuK-45Dp; Wed, 15 Oct 2025 11:02:14 +1100 Date: Wed, 15 Oct 2025 11:02:14 +1100 From: Dave Chinner To: Mateusz Guzik Cc: Jan Kara , brauner@kernel.org, viro@zeniv.linux.org.uk, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, josef@toxicpanda.com, kernel-team@fb.com, amir73il@gmail.com, linux-btrfs@vger.kernel.org, linux-ext4@vger.kernel.org, linux-xfs@vger.kernel.org, ceph-devel@vger.kernel.org, linux-unionfs@vger.kernel.org Subject: Re: [PATCH v7 13/14] xfs: use the new ->i_state accessors Message-ID: References: <20251009075929.1203950-1-mjguzik@gmail.com> <20251009075929.1203950-14-mjguzik@gmail.com> 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: On Fri, Oct 10, 2025 at 05:40:49PM +0200, Mateusz Guzik wrote: > On Fri, Oct 10, 2025 at 4:41 PM Jan Kara wrote: > > > > On Thu 09-10-25 09:59:27, Mateusz Guzik wrote: > > > Change generated with coccinelle and fixed up by hand as appropriate. > > > > > > Signed-off-by: Mateusz Guzik > > > > ... > > > > > @@ -2111,7 +2111,7 @@ xfs_rename_alloc_whiteout( > > > */ > > > xfs_setup_iops(tmpfile); > > > xfs_finish_inode_setup(tmpfile); > > > - VFS_I(tmpfile)->i_state |= I_LINKABLE; > > > + inode_state_set_raw(VFS_I(tmpfile), I_LINKABLE); > > > > > > *wip = tmpfile; > > > return 0; > > > @@ -2330,7 +2330,7 @@ xfs_rename( > > > * flag from the inode so it doesn't accidentally get misused in > > > * future. > > > */ > > > - VFS_I(du_wip.ip)->i_state &= ~I_LINKABLE; > > > + inode_state_clear_raw(VFS_I(du_wip.ip), I_LINKABLE); > > > } > > > > > > out_commit: > > > > These two accesses look fishy (not your fault but when we are doing this > > i_state exercise better make sure all the places are correct before > > papering over bugs with _raw function variant). How come they cannot race > > with other i_state modifications and thus corrupt i_state? > > > > I asked about this here: > https://lore.kernel.org/linux-xfs/CAGudoHEi05JGkTQ9PbM20D98S9fv0hTqpWRd5fWjEwkExSiVSw@mail.gmail.com/ Yes, as I said, we can add locking here if necessary, but locking isn't necessary at this point in time because nothing else can change the state of the newly allocated whiteout inode until we unlock it. Keep in mind the reason why we need I_LINKABLE here - it's not needed for correctness - it's needed to avoid a warning embedded in inc_nlink() because filesystems aren't trusted to implement link counts correctly anymore. Now we're being told that "it is too dangerous to let filesystems manage inode state themselves" and so we have to add extra overhead to code that we were forced to add to avoid VFS warnings added because the VFS doesn't trust filesystems to maintain some other important inode state.... So, if you want to get rid of XFS using I_LINKABLE here, please fix the nlink VFS api to allow us to call inc_nlink_() on a zero link inode without I_LINKABLE needing to be set. We do actually know what we are doing here, and as such needing I_LINKABLE here is nothing but a hacky workaround for inflexible, trustless VFS APIs... > > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c > > > index caff0125faea..ad94fbf55014 100644 > > > --- a/fs/xfs/xfs_iops.c > > > +++ b/fs/xfs/xfs_iops.c > > > @@ -1420,7 +1420,7 @@ xfs_setup_inode( > > > bool is_meta = xfs_is_internal_inode(ip); > > > > > > inode->i_ino = ip->i_ino; > > > - inode->i_state |= I_NEW; > > > + inode_state_set_raw(inode, I_NEW); "set" is wrong and will introduce a regression. This must be an "add" operation as inode->i_state may have already been modified by the time we get here. From 2021: commit f38a032b165d812b0ba8378a5cd237c0888ff65f Author: Dave Chinner Date: Tue Aug 24 19:13:04 2021 -0700 xfs: fix I_DONTCACHE Yup, the VFS hoist broke it, and nobody noticed. Bulkstat workloads make it clear that it doesn't work as it should. Fixes: dae2f8ed7992 ("fs: Lift XFS_IDONTCACHE to the VFS layer") Signed-off-by: Dave Chinner Reviewed-by: Darrick J. Wong Signed-off-by: Darrick J. Wong diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c index a3fe4c5307d3..f2210d927481 100644 --- a/fs/xfs/xfs_icache.c +++ b/fs/xfs/xfs_icache.c @@ -84,8 +84,9 @@ xfs_inode_alloc( return NULL; } - /* VFS doesn't initialise i_mode! */ + /* VFS doesn't initialise i_mode or i_state! */ VFS_I(ip)->i_mode = 0; + VFS_I(ip)->i_state = 0; XFS_STATS_INC(mp, vn_active); ASSERT(atomic_read(&ip->i_pincount) == 0); diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c index 0ff0cca94092..a607d6aca5c4 100644 --- a/fs/xfs/xfs_iops.c +++ b/fs/xfs/xfs_iops.c @@ -1344,7 +1344,7 @@ xfs_setup_inode( gfp_t gfp_mask; inode->i_ino = ip->i_ino; - inode->i_state = I_NEW; + inode->i_state |= I_NEW; inode_sb_list_add(inode); /* make the inode look hashed for the writeback code */ -Dave. -- Dave Chinner david@fromorbit.com