Received-SPF: pass (google.com: domain of linux-kernel+bounces-79464-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Fri, 23 Feb 2024 20:18:30 -0800
From: "Darrick J. Wong" <djwong@kernel.org>
To: John Garry <john.g.garry@oracle.com>
Cc: hch@lst.de, viro@zeniv.linux.org.uk, brauner@kernel.org,
	dchinner@redhat.com, jack@suse.cz, chandan.babu@oracle.com,
	martin.petersen@oracle.com, linux-kernel@vger.kernel.org,
	linux-xfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	tytso@mit.edu, jbongio@google.com, ojaswin@linux.ibm.com
Subject: Re: [PATCH 6/6] fs: xfs: Set FMODE_CAN_ATOMIC_WRITE for
 FS_XFLAG_ATOMICWRITES set
Message-ID: <20240224041830.GK6184@frogsfrogsfrogs>
References: <20240124142645.9334-1-john.g.garry@oracle.com>
 <20240124142645.9334-7-john.g.garry@oracle.com>
 <20240202180619.GK6184@frogsfrogsfrogs>
 <7e3b9556-f083-4c14-a48f-46242d1c744b@oracle.com>
 <20240213175954.GV616564@frogsfrogsfrogs>
 <b902bee1-fcfd-4542-8a4e-c6b9861828c9@oracle.com>
 <20240221170031.GI6184@frogsfrogsfrogs>
 <f7ad3aed-3482-4eee-8c81-8e471916ef82@oracle.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <f7ad3aed-3482-4eee-8c81-8e471916ef82@oracle.com>

On Wed, Feb 21, 2024 at 05:38:39PM +0000, John Garry wrote:
> On 21/02/2024 17:00, Darrick J. Wong wrote:
> > > > Hmm.  Well, if we move towards pushing all the hardware checks out of
> > > > xfs/iomap and into whatever goes on underneath submit_bio then I guess
> > > > we don't need to check device support here at all.
> > > Yeah, I have been thinking about this. But I was still planning on putting a
> > > "bdev on atomic write" check here, as you mentioned.
> > > 
> > > But is this a proper method to access the bdev for an xfs inode:
> > > 
> > > STATIC bool
> > > xfs_file_can_atomic_write(
> > > struct xfs_inode *inode)
> > > {
> > > 	struct xfs_buftarg *target = xfs_inode_buftarg(inode);
> > > 	struct block_device *bdev = target->bt_bdev;
> > > 
> > > 	if (!xfs_inode_atomicwrites(inode))
> > > 		return false;
> > > 
> > > 	return bdev_can_atomic_write(bdev);
> > > }
> > There's still a TOCTOU race problem if the bdev gets reconfigured
> > between xfs_file_can_atomic_write and submit_bio.
> 
> If that is the case then a check in the bio submit path is required to catch
> any such reconfigure problems - and we effectively have that in this series.
> 
> I am looking at change some of these XFS bdev_can_atomic_write() checks, but
> would still have a check in the bio submit path.

<nod> "check in the bio submit path" sounds good to me.  Adding in
redundant checks which are eventually gated on whatever submit_bio does
sounds like excessive overhead and layering violations.

> > 
> > However, if you're only using this to advertise the capability via statx
> > then I suppose that's fine -- userspace has to have some means of
> > discovering the ability at all.  Userspace is also inherently racy.
> > 
> > > I do notice the dax check in xfs_bmbt_to_iomap() when assigning iomap->bdev,
> > > which is creating some doubt?
> > Do you mean this?
> > 
> > 	if (mapping_flags & IOMAP_DAX)
> > 		iomap->dax_dev = target->bt_daxdev;
> > 	else
> > 		iomap->bdev = target->bt_bdev;
> > 
> > The dax path wants dax_dev set so that it can do the glorified memcpy
> > operation, and it doesn't need (or want) a block device.
> 
> Yes, so proper to use target->bt_bdev for checks for bdev atomic write
> capability, right?

Right.  fsdax doesn't support atomic memcpy to pmem.

--D

> 
> Thanks,
> John
> 
>