From: Dave Chinner <david@fromorbit.com>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: John Garry <john.g.garry@oracle.com>,
hch@lst.de, viro@zeniv.linux.org.uk, brauner@kernel.org,
jack@suse.cz, chandan.babu@oracle.com, willy@infradead.org,
axboe@kernel.dk, martin.petersen@oracle.com,
linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
tytso@mit.edu, jbongio@google.com, ojaswin@linux.ibm.com,
ritesh.list@gmail.com, mcgrof@kernel.org, p.raghav@samsung.com,
linux-xfs@vger.kernel.org, catherine.hoang@oracle.com
Subject: Re: [PATCH RFC v3 12/21] xfs: Only free full extents for forcealign
Date: Thu, 2 May 2024 13:12:33 +1000 [thread overview]
Message-ID: <ZjMEob4s3721orKp@dread.disaster.area> (raw)
In-Reply-To: <20240501235310.GP360919@frogsfrogsfrogs>
On Wed, May 01, 2024 at 04:53:10PM -0700, Darrick J. Wong wrote:
> On Wed, May 01, 2024 at 10:53:28AM +1000, Dave Chinner wrote:
> > On Mon, Apr 29, 2024 at 05:47:37PM +0000, John Garry wrote:
> > > Like we already do for rtvol, only free full extents for forcealign in
> > > xfs_free_file_space().
> > >
> > > Signed-off-by: John Garry <john.g.garry@oracle.com>
> > > ---
> > > fs/xfs/xfs_bmap_util.c | 7 +++++--
> > > 1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> > > index f26d1570b9bd..1dd45dfb2811 100644
> > > --- a/fs/xfs/xfs_bmap_util.c
> > > +++ b/fs/xfs/xfs_bmap_util.c
> > > @@ -847,8 +847,11 @@ xfs_free_file_space(
> > > startoffset_fsb = XFS_B_TO_FSB(mp, offset);
> > > endoffset_fsb = XFS_B_TO_FSBT(mp, offset + len);
> > >
> > > - /* We can only free complete realtime extents. */
> > > - if (XFS_IS_REALTIME_INODE(ip) && mp->m_sb.sb_rextsize > 1) {
> > > + /* Free only complete extents. */
> > > + if (xfs_inode_has_forcealign(ip) && ip->i_extsize > 1) {
> > > + startoffset_fsb = roundup_64(startoffset_fsb, ip->i_extsize);
> > > + endoffset_fsb = rounddown_64(endoffset_fsb, ip->i_extsize);
> > > + } else if (XFS_IS_REALTIME_INODE(ip) && mp->m_sb.sb_rextsize > 1) {
> > > startoffset_fsb = xfs_rtb_roundup_rtx(mp, startoffset_fsb);
> > > endoffset_fsb = xfs_rtb_rounddown_rtx(mp, endoffset_fsb);
> > > }
> >
> > When you look at xfs_rtb_roundup_rtx() you'll find it's just a one
> > line wrapper around roundup_64().
>
> I added this a couple of cycles ago to get ready for realtime
> modernization.
Yes, I know. I'm not suggesting that there's anything wrong with
this code, just pointing out that the RT wrappers are doing the
exact same conversion as the force-align code is doing. And from
that observation, a common implementation makes a lot of sense
because that same logic is repeated in quite a few places....
> That will create a bunch *more* churn in my tree just to
> convert everything *back*.
This doesn't change anything significant in your tree, nor do you
need to "convert everything back". The RT wrappers are unchanged,
and the only material difference in your tree vs the upstream
xfs_free_file_space() this patchset is based on is this:
- if (XFS_IS_REALTIME_INODE(ip) && mp->m_sb.sb_rextsize > 1) {
+ if (xfs_inode_has_bigrtalloc(ip)) {
That's it.
All the suggestion I made does is change where you need to make this
one line change. It would also remove the need to do this one line
change in multiple other places, so it would actually -reduce- your
ongoing rebase pain, not make it worse.
That's a net win for everyone, and it's most definitely not a reason
to shout at people and threaten to revert any changes they might
make in this area of the code.
> Where the hell were you when that was being reviewed?!!!
How is this sort of unhelpful statement in any way relevant to
improving the forcealign functionality to the point where we can
actually merge it and start making use of it for atomic writes?
-Dave.
--
Dave Chinner
david@fromorbit.com
next prev parent reply other threads:[~2024-05-02 3:12 UTC|newest]
Thread overview: 58+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-29 17:47 [PATCH v3 00/21] block atomic writes for XFS John Garry
2024-04-29 17:47 ` [PATCH v3 01/21] fs: Add generic_atomic_write_valid_size() John Garry
2024-04-29 17:47 ` [PATCH v3 02/21] xfs: only allow minlen allocations when near ENOSPC John Garry
2024-04-29 17:47 ` [PATCH v3 03/21] xfs: always tail align maxlen allocations John Garry
2024-04-29 17:47 ` [PATCH v3 04/21] xfs: simplify extent allocation alignment John Garry
2024-04-29 17:47 ` [PATCH v3 05/21] xfs: make EOF allocation simpler John Garry
2024-04-29 17:47 ` [PATCH v3 06/21] xfs: introduce forced allocation alignment John Garry
2024-04-29 17:47 ` [PATCH v3 07/21] fs: xfs: align args->minlen for " John Garry
2024-06-05 14:26 ` John Garry
2024-06-06 8:47 ` Dave Chinner
2024-06-06 16:22 ` John Garry
2024-06-07 6:04 ` John Garry
2024-04-29 17:47 ` [PATCH v3 08/21] xfs: Introduce FORCEALIGN inode flag John Garry
2024-04-30 23:22 ` Dave Chinner
2024-05-01 10:03 ` John Garry
2024-05-02 0:50 ` Dave Chinner
2024-05-02 7:56 ` John Garry
2024-06-12 2:10 ` Long Li
2024-06-12 6:55 ` John Garry
2024-04-29 17:47 ` [PATCH v3 09/21] xfs: Do not free EOF blocks for forcealign John Garry
2024-04-30 22:54 ` Dave Chinner
2024-05-01 8:30 ` John Garry
2024-05-02 1:11 ` Dave Chinner
2024-05-02 8:55 ` John Garry
2024-04-29 17:47 ` [PATCH v3 10/21] xfs: Update xfs_is_falloc_aligned() mask " John Garry
2024-04-30 23:35 ` Dave Chinner
2024-05-01 10:48 ` John Garry
2024-05-01 23:45 ` Darrick J. Wong
2024-04-29 17:47 ` [PATCH RFC v3 11/21] xfs: Unmap blocks according to forcealign John Garry
2024-05-01 0:10 ` Dave Chinner
2024-05-01 10:54 ` John Garry
2024-06-06 9:50 ` John Garry
2024-04-29 17:47 ` [PATCH RFC v3 12/21] xfs: Only free full extents for forcealign John Garry
2024-05-01 0:53 ` Dave Chinner
2024-05-01 11:24 ` John Garry
2024-05-01 23:53 ` Darrick J. Wong
2024-05-02 3:12 ` Dave Chinner [this message]
2024-04-29 17:47 ` [PATCH v3 13/21] xfs: Enable file data forcealign feature John Garry
2024-04-29 17:47 ` [PATCH v3 14/21] iomap: Sub-extent zeroing John Garry
2024-05-01 1:07 ` Dave Chinner
2024-05-01 10:23 ` John Garry
2024-05-30 10:40 ` John Garry
2024-06-11 3:10 ` Long Li
2024-06-11 7:29 ` John Garry
2024-04-29 17:47 ` [PATCH v3 15/21] fs: xfs: " John Garry
2024-05-01 1:32 ` Dave Chinner
2024-05-01 11:36 ` John Garry
2024-05-02 1:26 ` Dave Chinner
2024-04-29 17:47 ` [PATCH v3 16/21] fs: Add FS_XFLAG_ATOMICWRITES flag John Garry
2024-04-29 17:47 ` [PATCH v3 17/21] iomap: Atomic write support John Garry
2024-05-01 1:47 ` Dave Chinner
2024-05-01 11:08 ` John Garry
2024-05-02 1:43 ` Dave Chinner
2024-05-02 9:12 ` John Garry
2024-04-29 17:47 ` [PATCH v3 18/21] xfs: Support FS_XFLAG_ATOMICWRITES for forcealign John Garry
2024-04-29 17:47 ` [PATCH v3 19/21] xfs: Support atomic write for statx John Garry
2024-04-29 17:47 ` [PATCH v3 20/21] xfs: Validate atomic writes John Garry
2024-04-29 17:47 ` [PATCH v3 21/21] xfs: Support setting FMODE_CAN_ATOMIC_WRITE John Garry
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=ZjMEob4s3721orKp@dread.disaster.area \
--to=david@fromorbit.com \
--cc=axboe@kernel.dk \
--cc=brauner@kernel.org \
--cc=catherine.hoang@oracle.com \
--cc=chandan.babu@oracle.com \
--cc=djwong@kernel.org \
--cc=hch@lst.de \
--cc=jack@suse.cz \
--cc=jbongio@google.com \
--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=martin.petersen@oracle.com \
--cc=mcgrof@kernel.org \
--cc=ojaswin@linux.ibm.com \
--cc=p.raghav@samsung.com \
--cc=ritesh.list@gmail.com \
--cc=tytso@mit.edu \
--cc=viro@zeniv.linux.org.uk \
--cc=willy@infradead.org \
/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;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).