diff mbox series

[5.10,v3,1/5] xfs: remove infinite loop when reserving free block pool

Message ID 20220901133356.2473299-2-amir73il@gmail.com (mailing list archive)
State Deferred, archived
Headers show
Series xfs stable patches for 5.10.y (from v5.18+) | expand

Commit Message

Amir Goldstein Sept. 1, 2022, 1:33 p.m. UTC 
commit 15f04fdc75aaaa1cccb0b8b3af1be290e118a7bc upstream.

[Added wrapper xfs_fdblocks_unavailable() for 5.10.y backport]

Infinite loops in kernel code are scary.  Calls to xfs_reserve_blocks
should be rare (people should just use the defaults!) so we really don't
need to try so hard.  Simplify the logic here by removing the infinite
loop.

Cc: Brian Foster <bfoster@redhat.com>
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Acked-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_fsops.c | 52 +++++++++++++++++++---------------------------
 fs/xfs/xfs_mount.h |  8 +++++++
 2 files changed, 29 insertions(+), 31 deletions(-)

Comments

Greg Kroah-Hartman Sept. 2, 2022, 6:16 a.m. UTC | #1 
On Thu, Sep 01, 2022 at 04:33:52PM +0300, Amir Goldstein wrote:
> commit 15f04fdc75aaaa1cccb0b8b3af1be290e118a7bc upstream.
> 
> [Added wrapper xfs_fdblocks_unavailable() for 5.10.y backport]

You forgot the correct Author/From: information here :(

Please be more careful next time.

greg k-h
Amir Goldstein Sept. 2, 2022, 7:04 a.m. UTC | #2 
On Fri, Sep 2, 2022 at 9:16 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Sep 01, 2022 at 04:33:52PM +0300, Amir Goldstein wrote:
> > commit 15f04fdc75aaaa1cccb0b8b3af1be290e118a7bc upstream.
> >
> > [Added wrapper xfs_fdblocks_unavailable() for 5.10.y backport]
>
> You forgot the correct Author/From: information here :(
>
> Please be more careful next time.
>

Sorry.
I noticed that happens from time to time and sometimes
I catch that.
I wonder why/when git has done that. during cherry-pick? rebase?
I certainly did not do --reset-author at any point.

I might have added an original commit adding the Accessor
and then squashed it in rebase. That may be the reason.

I'll remember to add the From: check to my pre-post eyeballing.

Thanks,
Amir.
diff mbox series

Patch

diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index ef1d5bb88b93..6d4f4271e7be 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -376,46 +376,36 @@  xfs_reserve_blocks(
 	 * If the request is larger than the current reservation, reserve the
 	 * blocks before we update the reserve counters. Sample m_fdblocks and
 	 * perform a partial reservation if the request exceeds free space.
+	 *
+	 * The code below estimates how many blocks it can request from
+	 * fdblocks to stash in the reserve pool.  This is a classic TOCTOU
+	 * race since fdblocks updates are not always coordinated via
+	 * m_sb_lock.
 	 */
-	error = -ENOSPC;
-	do {
-		free = percpu_counter_sum(&mp->m_fdblocks) -
-						mp->m_alloc_set_aside;
-		if (free <= 0)
-			break;
-
-		delta = request - mp->m_resblks;
-		lcounter = free - delta;
-		if (lcounter < 0)
-			/* We can't satisfy the request, just get what we can */
-			fdblks_delta = free;
-		else
-			fdblks_delta = delta;
-
+	free = percpu_counter_sum(&mp->m_fdblocks) -
+						xfs_fdblocks_unavailable(mp);
+	delta = request - mp->m_resblks;
+	if (delta > 0 && free > 0) {
 		/*
 		 * We'll either succeed in getting space from the free block
-		 * count or we'll get an ENOSPC. If we get a ENOSPC, it means
-		 * things changed while we were calculating fdblks_delta and so
-		 * we should try again to see if there is anything left to
-		 * reserve.
-		 *
-		 * Don't set the reserved flag here - we don't want to reserve
-		 * the extra reserve blocks from the reserve.....
+		 * count or we'll get an ENOSPC.  Don't set the reserved flag
+		 * here - we don't want to reserve the extra reserve blocks
+		 * from the reserve.
 		 */
+		fdblks_delta = min(free, delta);
 		spin_unlock(&mp->m_sb_lock);
 		error = xfs_mod_fdblocks(mp, -fdblks_delta, 0);
 		spin_lock(&mp->m_sb_lock);
-	} while (error == -ENOSPC);
 
-	/*
-	 * Update the reserve counters if blocks have been successfully
-	 * allocated.
-	 */
-	if (!error && fdblks_delta) {
-		mp->m_resblks += fdblks_delta;
-		mp->m_resblks_avail += fdblks_delta;
+		/*
+		 * Update the reserve counters if blocks have been successfully
+		 * allocated.
+		 */
+		if (!error) {
+			mp->m_resblks += fdblks_delta;
+			mp->m_resblks_avail += fdblks_delta;
+		}
 	}
-
 out:
 	if (outval) {
 		outval->resblks = mp->m_resblks;
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index dfa429b77ee2..3a6bc9dc11b5 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -406,6 +406,14 @@  extern int	xfs_initialize_perag(xfs_mount_t *mp, xfs_agnumber_t agcount,
 				     xfs_agnumber_t *maxagi);
 extern void	xfs_unmountfs(xfs_mount_t *);
 
+/* Accessor added for 5.10.y backport */
+static inline uint64_t
+xfs_fdblocks_unavailable(
+	struct xfs_mount	*mp)
+{
+	return mp->m_alloc_set_aside;
+}
+
 extern int	xfs_mod_fdblocks(struct xfs_mount *mp, int64_t delta,
 				 bool reserved);
 extern int	xfs_mod_frextents(struct xfs_mount *mp, int64_t delta);