linux-kernel - Re: [PATCH 11/12] mm/filemap: make buffered writes work with RWF

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 for Android: free password hash cracker in your pocket

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

Message-ID: <Z1gmk_X9RrG7O0Fi@infradead.org>
Date: Tue, 10 Dec 2024 03:31:31 -0800
From: Christoph Hellwig <hch@...radead.org>
To: Jens Axboe <axboe@...nel.dk>
Cc: "Darrick J. Wong" <djwong@...nel.org>, linux-mm@...ck.org,
	linux-fsdevel@...r.kernel.org, hannes@...xchg.org, clm@...a.com,
	linux-kernel@...r.kernel.org, willy@...radead.org,
	kirill@...temov.name, bfoster@...hat.com
Subject: Re: [PATCH 11/12] mm/filemap: make buffered writes work with
 RWF_UNCACHED

On Fri, Dec 06, 2024 at 11:22:55AM -0700, Jens Axboe wrote:
> > Honestly, I'm not a fan of foliop_uncached or foliop_is_uncached.
> 
> It definitely is what I would elegantly refer to as somewhat of a
> hack... But it's not _that_ bad imho.

It's pretty horrible actually.

> > I think these two macros are only used for ext4 (or really, !iomap)
> > support, right?  And that's only to avoid messing with ->write_begin?
> 
> Indeed, ideally we'd change ->write_begin() instead. And that probably
> should still be done, I just did not want to deal with that nightmare in
> terms of managing the patchset. And honestly I think it'd be OK to defer
> that part until ->write_begin() needs to be changed for other reasons,
> it's a lot of churn just for this particular thing and dealing with the
> magic pointer value (at least to me) is liveable.

->write_begin() really should just go away, it is a horrible interface.
Note that in that past it actually had a flags argument, but that got
killed a while ago.

> > What if you dropped ext4 support instead? :D
> 
> Hah, yes obviously that'd be a solution, then I'd need to drop btrfs as
> well. And I would kind of prefer not doing that ;-)

Btrfs doesn't need it.  In fact the code would be cleaner and do less
work with out, see the patch below.  And for ext4 there already is an
iomap conversion patch series on the list that just needs more review,
so skipping it here and growing the uncached support through that sounds
sensible.

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 444bee219b4e..db3a3c7ecf77 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -894,20 +894,17 @@ static gfp_t get_prepare_gfp_flags(struct inode *inode, bool nowait)
  */
 static noinline int prepare_one_folio(struct inode *inode, struct folio **folio_ret,
 				      loff_t pos, size_t write_bytes,
-				      bool force_uptodate, bool nowait)
+				      bool force_uptodate, fgf_t fgp_flags)
 {
 	unsigned long index = pos >> PAGE_SHIFT;
-	gfp_t mask = get_prepare_gfp_flags(inode, nowait);
-	fgf_t fgp_flags = (nowait ? FGP_WRITEBEGIN | FGP_NOWAIT : FGP_WRITEBEGIN);
+	gfp_t mask = get_prepare_gfp_flags(inode, fgp_flags & FGP_NOWAIT);
 	struct folio *folio;
 	int ret = 0;
 
-	if (foliop_is_uncached(folio_ret))
-		fgp_flags |= FGP_UNCACHED;
 again:
 	folio = __filemap_get_folio(inode->i_mapping, index, fgp_flags, mask);
 	if (IS_ERR(folio)) {
-		if (nowait)
+		if (fgp_flags & FGP_NOWAIT)
 			ret = -EAGAIN;
 		else
 			ret = PTR_ERR(folio);
@@ -925,7 +922,7 @@ static noinline int prepare_one_folio(struct inode *inode, struct folio **folio_
 	if (ret) {
 		/* The folio is already unlocked. */
 		folio_put(folio);
-		if (!nowait && ret == -EAGAIN) {
+		if (!(fgp_flags & FGP_NOWAIT) && ret == -EAGAIN) {
 			ret = 0;
 			goto again;
 		}
@@ -1135,9 +1132,15 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
 	const bool nowait = (iocb->ki_flags & IOCB_NOWAIT);
 	unsigned int bdp_flags = (nowait ? BDP_ASYNC : 0);
 	bool only_release_metadata = false;
+	fgf_t fgp_flags = FGP_WRITEBEGIN;
 
-	if (nowait)
+	if (nowait) {
 		ilock_flags |= BTRFS_ILOCK_TRY;
+		fgp_flags |= FGP_NOWAIT;
+	}
+
+	if (iocb->ki_flags & IOCB_UNCACHED)
+		fgp_flags |= FGP_UNCACHED;
 
 	ret = btrfs_inode_lock(BTRFS_I(inode), ilock_flags);
 	if (ret < 0)
@@ -1232,11 +1235,8 @@ ssize_t btrfs_buffered_write(struct kiocb *iocb, struct iov_iter *i)
 			break;
 		}
 
-		if (iocb->ki_flags & IOCB_UNCACHED)
-			folio = foliop_uncached;
-
 		ret = prepare_one_folio(inode, &folio, pos, write_bytes,
-					force_page_uptodate, false);
+					force_page_uptodate, fgp_flags);
 		if (ret) {
 			btrfs_delalloc_release_extents(BTRFS_I(inode),
 						       reserve_bytes);