mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Dave Chinner <david@fromorbit.com>
To: Zhang Yi <yi.zhang@huaweicloud.com>
Cc: linux-xfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-kernel@vger.kernel.org, djwong@kernel.org,
	hch@infradead.org, brauner@kernel.org, chandanbabu@kernel.org,
	John Garry <john.g.garry@oracle.com>,
	jack@suse.cz, yi.zhang@huawei.com, chengzhihao1@huawei.com,
	yukuai3@huawei.com
Subject: Re: [PATCH -next v6 1/2] xfs: reserve blocks for truncating large realtime inode
Date: Mon, 1 Jul 2024 22:35:07 +1000	[thread overview]
Message-ID: <ZoKie9aZV0sHIbA8@dread.disaster.area> (raw)
In-Reply-To: <b27977d3-3764-886d-7067-483cea203fbe@huaweicloud.com>

On Mon, Jul 01, 2024 at 10:26:18AM +0800, Zhang Yi wrote:
> On 2024/7/1 9:16, Dave Chinner wrote:
> > On Tue, Jun 18, 2024 at 10:21:11PM +0800, Zhang Yi wrote:
> >> @@ -917,7 +920,17 @@ xfs_setattr_size(
> >>  			return error;
> >>  	}
> >>  
> >> -	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp);
> >> +	/*
> >> +	 * For realtime inode with more than one block rtextsize, we need the
> >> +	 * block reservation for bmap btree block allocations/splits that can
> >> +	 * happen since it could split the tail written extent and convert the
> >> +	 * right beyond EOF one to unwritten.
> >> +	 */
> >> +	if (xfs_inode_has_bigrtalloc(ip))
> >> +		resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
> > 
> > .... should this be doing this generic check instead:
> > 
> > 	if (xfs_inode_alloc_unitsize(ip) > 1)
> 
>         if (xfs_inode_alloc_unitsize(ip) > i_blocksize(inode)) ?
> 
> > 		resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
> > 
> 
> Yeah, it makes sense to me, but Christoph suggested to think about force
> aligned allocations later, so I only dealt with the big RT inode case here.
> I can revise it if John and Christoph don't object.

Sorry, but I don't really care what either John or Christoph say on
this matter: xfs_inode_has_bigrtalloc() is recently introduced
technical debt that should not be propagated further.

xfs_inode_has_bigrtalloc() needs to be replaced completely with
xfs_inode_alloc_unitsize() and any conditional behaviour needed can
be based on the return value from xfs_inode_alloc_unitsize(). That
works for everything that has an allocation block size larger than
one filesystem block, not just one specific RT case.

Don't force John to have fix all these same RT bugs that are being
fixed with xfs_inode_has_bigrtalloc() just because forced alignment
stuff is not yet merged. Don't make John's life harder than it needs
to be to get that stuff merged, and don't waste my time arguing
about it: just fix the problem the right way the first time.

-Dave.
-- 
Dave Chinner
david@fromorbit.com

  reply	other threads:[~2024-07-01 12:35 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-06-18 14:21 [PATCH -next v6 0/2] iomap/xfs: fix stale data exposure when truncating realtime inodes Zhang Yi
2024-06-18 14:21 ` [PATCH -next v6 1/2] xfs: reserve blocks for truncating large realtime inode Zhang Yi
2024-07-01  1:16   ` Dave Chinner
2024-07-01  2:26     ` Zhang Yi
2024-07-01 12:35       ` Dave Chinner [this message]
2024-07-02  7:34         ` Christoph Hellwig
2024-06-18 14:21 ` [PATCH -next v6 2/2] iomap: don't increase i_size in iomap_write_end() Zhang Yi
2024-06-19  5:57   ` Christoph Hellwig
2024-06-19  0:24 ` [PATCH -next v6 0/2] iomap/xfs: fix stale data exposure when truncating realtime inodes Darrick J. Wong
2024-06-19  1:52   ` Zhang Yi
2024-06-19 14:01 ` Christian Brauner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZoKie9aZV0sHIbA8@dread.disaster.area \
    --to=david@fromorbit.com \
    --cc=brauner@kernel.org \
    --cc=chandanbabu@kernel.org \
    --cc=chengzhihao1@huawei.com \
    --cc=djwong@kernel.org \
    --cc=hch@infradead.org \
    --cc=jack@suse.cz \
    --cc=john.g.garry@oracle.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-xfs@vger.kernel.org \
    --cc=yi.zhang@huawei.com \
    --cc=yi.zhang@huaweicloud.com \
    --cc=yukuai3@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®