linux-kernel - Re: [PATCH v3 15/21] fs: xfs: iomap: Sub-extent zeroing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 2 May 2024 11:26:47 +1000
From: Dave Chinner <david@...morbit.com>
To: John Garry <john.g.garry@...cle.com>
Cc: djwong@...nel.org, hch@....de, viro@...iv.linux.org.uk,
	brauner@...nel.org, jack@...e.cz, chandan.babu@...cle.com,
	willy@...radead.org, axboe@...nel.dk, martin.petersen@...cle.com,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	tytso@....edu, jbongio@...gle.com, ojaswin@...ux.ibm.com,
	ritesh.list@...il.com, mcgrof@...nel.org, p.raghav@...sung.com,
	linux-xfs@...r.kernel.org, catherine.hoang@...cle.com
Subject: Re: [PATCH v3 15/21] fs: xfs: iomap: Sub-extent zeroing

On Wed, May 01, 2024 at 12:36:02PM +0100, John Garry wrote:
> On 01/05/2024 02:32, Dave Chinner wrote:
> > On Mon, Apr 29, 2024 at 05:47:40PM +0000, John Garry wrote:
> > > Set iomap->extent_size when sub-extent zeroing is required.
> > > 
> > > We treat a sub-extent write same as an unaligned write, so we can leverage
> > > the existing sub-FSblock unaligned write support, i.e. try a shared lock
> > > with IOMAP_DIO_OVERWRITE_ONLY flag, if this fails then try the exclusive
> > > lock.
> > > 
> > > In xfs_iomap_write_unwritten(), FSB calcs are now based on the extsize.
> > 
> > If forcedalign is set, should we just reject unaligned DIOs?
> 
> Why would we? That's very restrictive. Indeed, we got to the point of adding
> the sub-extent zeroing just for supporting that.
> > > @@ -646,9 +647,9 @@ xfs_file_dio_write_unaligned(
> > >   	ssize_t			ret;
> > >   	/*
> > > -	 * Extending writes need exclusivity because of the sub-block zeroing
> > > -	 * that the DIO code always does for partial tail blocks beyond EOF, so
> > > -	 * don't even bother trying the fast path in this case.
> > > +	 * Extending writes need exclusivity because of the sub-block/extent
> > > +	 * zeroing that the DIO code always does for partial tail blocks
> > > +	 * beyond EOF, so don't even bother trying the fast path in this case.
> > >   	 */
> > >   	if (iocb->ki_pos > isize || iocb->ki_pos + count >= isize) {
> > >   		if (iocb->ki_flags & IOCB_NOWAIT)
> > > @@ -714,11 +715,19 @@ xfs_file_dio_write(
> > >   	struct xfs_inode	*ip = XFS_I(file_inode(iocb->ki_filp));
> > >   	struct xfs_buftarg      *target = xfs_inode_buftarg(ip);
> > >   	size_t			count = iov_iter_count(from);
> > > +	struct xfs_mount	*mp = ip->i_mount;
> > > +	unsigned int		blockmask;
> > >   	/* direct I/O must be aligned to device logical sector size */
> > >   	if ((iocb->ki_pos | count) & target->bt_logical_sectormask)
> > >   		return -EINVAL;
> > > -	if ((iocb->ki_pos | count) & ip->i_mount->m_blockmask)
> > > +
> > > +	if (xfs_inode_has_forcealign(ip) && ip->i_extsize > 1)
> > > +		blockmask = XFS_FSB_TO_B(mp, ip->i_extsize) - 1;
> > > +	else
> > > +		blockmask = mp->m_blockmask;
> > 
> > 	alignmask = XFS_FSB_TO_B(mp, xfs_inode_alignment(ip)) - 1;
> 
> Do you mean xfs_extent_alignment() instead of xfs_inode_alignment()?

Yes, I was.

I probably should have named it xfs_inode_extent_alignment() because
clearly I kept thinking of it as "inode alignment"... :)

-Dave.
-- 
Dave Chinner
david@...morbit.com