[PATCH v3 0/4] xfs: support shrinking free space in the last AG

[PATCH v3 0/4] xfs: support shrinking free space in the last AG

[Gao Xiang]

Hi folks,

This is version 3 of
https://lore.kernel.org/r/20201028231353.640969-1-hsiangkao@redhat.com

I resend to make sure if the shrinking upstreaming development can be
done in an incremental progress.

Days ago I also made a shrinking the entire AGs prototype at,
https://git.kernel.org/pub/scm/linux/kernel/git/xiang/linux.git/log/?h=xfs/shrink2
which is still WIP / rather incomplete, yet any directions/suggestions
about that would be greatly helpful to me as well.

This mainly focuses on the previous review from Brian. Mainly to seperate
the previous patch into a patchset. I'm not sure if it looks good this
time (yet I think [PATCH 4/4] is simple enough to address the shrinking
functionality) so post it again to reconfirm that.... and also to confirm
if we need to use #ifdef DEBUG to wrap up the entrance (Although my
humble opinion is that most end users don't build with DEBUG enabled....)

xfsprogs: https://lore.kernel.org/r/20201028114010.545331-1-hsiangkao@redhat.com
xfstests: https://lore.kernel.org/r/20201028230909.639698-1-hsiangkao@redhat.com

Changes since v2:
 - [PATCH 4/4] fix a bug about "if (extend && ...)" (Brian);
 - split out the original patch (Brian, Eric, Darrick).

Thanks,
Gao Xiang

Gao Xiang (4):
  xfs: rename `new' to `delta' in xfs_growfs_data_private()
  xfs: get rid of xfs_growfs_{data,log}_t
  xfs: hoist out xfs_resizefs_init_new_ags()
  xfs: support shrinking unused space in the last AG

 fs/xfs/libxfs/xfs_ag.c |  72 ++++++++++++++++++
 fs/xfs/libxfs/xfs_ag.h |   2 +
 fs/xfs/libxfs/xfs_fs.h |   4 +-
 fs/xfs/xfs_fsops.c     | 161 +++++++++++++++++++++++++++--------------
 fs/xfs/xfs_fsops.h     |   4 +-
 fs/xfs/xfs_ioctl.c     |   4 +-
 fs/xfs/xfs_trans.c     |   1 -
 7 files changed, 185 insertions(+), 63 deletions(-)

-- 
2.27.0

[PATCH v3 1/4] xfs: rename `new' to `delta' in xfs_growfs_data_private()

[Gao Xiang]

It actually means the delta block count of growfs. Rename it in order
to make it clear.

Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
 fs/xfs/xfs_fsops.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index 5870db855e8b..d254588f6e21 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -33,7 +33,7 @@ xfs_growfs_data_private(
 	xfs_agnumber_t		nagcount;
 	xfs_agnumber_t		nagimax = 0;
 	xfs_rfsblock_t		nb, nb_mod;
-	xfs_rfsblock_t		new;
+	xfs_rfsblock_t		delta;
 	xfs_agnumber_t		oagcount;
 	xfs_trans_t		*tp;
 	struct aghdr_init_data	id = {};
@@ -50,16 +50,16 @@ xfs_growfs_data_private(
 		return error;
 	xfs_buf_relse(bp);
 
-	new = nb;	/* use new as a temporary here */
-	nb_mod = do_div(new, mp->m_sb.sb_agblocks);
-	nagcount = new + (nb_mod != 0);
+	delta = nb;	/* use delta as a temporary here */
+	nb_mod = do_div(delta, mp->m_sb.sb_agblocks);
+	nagcount = delta + (nb_mod != 0);
 	if (nb_mod && nb_mod < XFS_MIN_AG_BLOCKS) {
 		nagcount--;
 		nb = (xfs_rfsblock_t)nagcount * mp->m_sb.sb_agblocks;
 		if (nb < mp->m_sb.sb_dblocks)
 			return -EINVAL;
 	}
-	new = nb - mp->m_sb.sb_dblocks;
+	delta = nb - mp->m_sb.sb_dblocks;
 	oagcount = mp->m_sb.sb_agcount;
 
 	/* allocate the new per-ag structures */
@@ -89,7 +89,7 @@ xfs_growfs_data_private(
 	INIT_LIST_HEAD(&id.buffer_list);
 	for (id.agno = nagcount - 1;
 	     id.agno >= oagcount;
-	     id.agno--, new -= id.agsize) {
+	     id.agno--, delta -= id.agsize) {
 
 		if (id.agno == nagcount - 1)
 			id.agsize = nb -
@@ -110,8 +110,8 @@ xfs_growfs_data_private(
 	xfs_trans_agblocks_delta(tp, id.nfree);
 
 	/* If there are new blocks in the old last AG, extend it. */
-	if (new) {
-		error = xfs_ag_extend_space(mp, tp, &id, new);
+	if (delta) {
+		error = xfs_ag_extend_space(mp, tp, &id, delta);
 		if (error)
 			goto out_trans_cancel;
 	}
@@ -143,7 +143,7 @@ xfs_growfs_data_private(
 	 * If we expanded the last AG, free the per-AG reservation
 	 * so we can reinitialize it with the new size.
 	 */
-	if (new) {
+	if (delta) {
 		struct xfs_perag	*pag;
 
 		pag = xfs_perag_get(mp, id.agno);
-- 
2.27.0

Re: [PATCH v3 1/4] xfs: rename `new' to `delta' in xfs_growfs_data_private()

[Darrick J. Wong]

On Sat, Jan 09, 2021 at 03:09:16AM +0800, Gao Xiang wrote:
> It actually means the delta block count of growfs. Rename it in order
> to make it clear.
> 
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
>  fs/xfs/xfs_fsops.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index 5870db855e8b..d254588f6e21 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/fs/xfs/xfs_fsops.c
> @@ -33,7 +33,7 @@ xfs_growfs_data_private(
>  	xfs_agnumber_t		nagcount;
>  	xfs_agnumber_t		nagimax = 0;
>  	xfs_rfsblock_t		nb, nb_mod;
> -	xfs_rfsblock_t		new;
> +	xfs_rfsblock_t		delta;
>  	xfs_agnumber_t		oagcount;
>  	xfs_trans_t		*tp;
>  	struct aghdr_init_data	id = {};
> @@ -50,16 +50,16 @@ xfs_growfs_data_private(
>  		return error;
>  	xfs_buf_relse(bp);
>  
> -	new = nb;	/* use new as a temporary here */
> -	nb_mod = do_div(new, mp->m_sb.sb_agblocks);
> -	nagcount = new + (nb_mod != 0);
> +	delta = nb;	/* use delta as a temporary here */

Yikes, can ... oh right, I complained about this in patch 4.

delta should probably be an int64_t, and this division/mod should have
its own temporary variable.

--D

> +	nb_mod = do_div(delta, mp->m_sb.sb_agblocks);
> +	nagcount = delta + (nb_mod != 0);
>  	if (nb_mod && nb_mod < XFS_MIN_AG_BLOCKS) {
>  		nagcount--;
>  		nb = (xfs_rfsblock_t)nagcount * mp->m_sb.sb_agblocks;
>  		if (nb < mp->m_sb.sb_dblocks)
>  			return -EINVAL;
>  	}
> -	new = nb - mp->m_sb.sb_dblocks;
> +	delta = nb - mp->m_sb.sb_dblocks;
>  	oagcount = mp->m_sb.sb_agcount;
>  
>  	/* allocate the new per-ag structures */
> @@ -89,7 +89,7 @@ xfs_growfs_data_private(
>  	INIT_LIST_HEAD(&id.buffer_list);
>  	for (id.agno = nagcount - 1;
>  	     id.agno >= oagcount;
> -	     id.agno--, new -= id.agsize) {
> +	     id.agno--, delta -= id.agsize) {
>  
>  		if (id.agno == nagcount - 1)
>  			id.agsize = nb -
> @@ -110,8 +110,8 @@ xfs_growfs_data_private(
>  	xfs_trans_agblocks_delta(tp, id.nfree);
>  
>  	/* If there are new blocks in the old last AG, extend it. */
> -	if (new) {
> -		error = xfs_ag_extend_space(mp, tp, &id, new);
> +	if (delta) {
> +		error = xfs_ag_extend_space(mp, tp, &id, delta);
>  		if (error)
>  			goto out_trans_cancel;
>  	}
> @@ -143,7 +143,7 @@ xfs_growfs_data_private(
>  	 * If we expanded the last AG, free the per-AG reservation
>  	 * so we can reinitialize it with the new size.
>  	 */
> -	if (new) {
> +	if (delta) {
>  		struct xfs_perag	*pag;
>  
>  		pag = xfs_perag_get(mp, id.agno);
> -- 
> 2.27.0
> 

[PATCH v3 2/4] xfs: get rid of xfs_growfs_{data,log}_t

[Gao Xiang]

Such usage isn't encouraged by the kernel coding style.

Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
 fs/xfs/libxfs/xfs_fs.h |  4 ++--
 fs/xfs/xfs_fsops.c     | 12 ++++++------
 fs/xfs/xfs_fsops.h     |  4 ++--
 fs/xfs/xfs_ioctl.c     |  4 ++--
 4 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
index 2a2e3cfd94f0..a17313efc1fe 100644
--- a/fs/xfs/libxfs/xfs_fs.h
+++ b/fs/xfs/libxfs/xfs_fs.h
@@ -308,12 +308,12 @@ struct xfs_ag_geometry {
 typedef struct xfs_growfs_data {
 	__u64		newblocks;	/* new data subvol size, fsblocks */
 	__u32		imaxpct;	/* new inode space percentage limit */
-} xfs_growfs_data_t;
+};
 
 typedef struct xfs_growfs_log {
 	__u32		newblocks;	/* new log size, fsblocks */
 	__u32		isint;		/* 1 if new log is internal */
-} xfs_growfs_log_t;
+};
 
 typedef struct xfs_growfs_rt {
 	__u64		newblocks;	/* new realtime size, fsblocks */
diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index d254588f6e21..6c5f6a50da2e 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -25,8 +25,8 @@
  */
 static int
 xfs_growfs_data_private(
-	xfs_mount_t		*mp,		/* mount point for filesystem */
-	xfs_growfs_data_t	*in)		/* growfs data input struct */
+	struct xfs_mount	*mp,		/* mount point for filesystem */
+	struct xfs_growfs_data	*in)		/* growfs data input struct */
 {
 	struct xfs_buf		*bp;
 	int			error;
@@ -35,7 +35,7 @@ xfs_growfs_data_private(
 	xfs_rfsblock_t		nb, nb_mod;
 	xfs_rfsblock_t		delta;
 	xfs_agnumber_t		oagcount;
-	xfs_trans_t		*tp;
+	struct xfs_trans	*tp;
 	struct aghdr_init_data	id = {};
 
 	nb = in->newblocks;
@@ -170,8 +170,8 @@ xfs_growfs_data_private(
 
 static int
 xfs_growfs_log_private(
-	xfs_mount_t		*mp,	/* mount point for filesystem */
-	xfs_growfs_log_t	*in)	/* growfs log input struct */
+	struct xfs_mount	*mp,	/* mount point for filesystem */
+	struct xfs_growfs_log	*in)	/* growfs log input struct */
 {
 	xfs_extlen_t		nb;
 
@@ -268,7 +268,7 @@ xfs_growfs_data(
 int
 xfs_growfs_log(
 	xfs_mount_t		*mp,
-	xfs_growfs_log_t	*in)
+	struct xfs_growfs_log	*in)
 {
 	int error;
 
diff --git a/fs/xfs/xfs_fsops.h b/fs/xfs/xfs_fsops.h
index 92869f6ec8d3..d7e9af4a28eb 100644
--- a/fs/xfs/xfs_fsops.h
+++ b/fs/xfs/xfs_fsops.h
@@ -6,8 +6,8 @@
 #ifndef __XFS_FSOPS_H__
 #define	__XFS_FSOPS_H__
 
-extern int xfs_growfs_data(xfs_mount_t *mp, xfs_growfs_data_t *in);
-extern int xfs_growfs_log(xfs_mount_t *mp, xfs_growfs_log_t *in);
+extern int xfs_growfs_data(xfs_mount_t *mp, struct xfs_growfs_data *in);
+extern int xfs_growfs_log(xfs_mount_t *mp, struct xfs_growfs_log *in);
 extern void xfs_fs_counts(xfs_mount_t *mp, xfs_fsop_counts_t *cnt);
 extern int xfs_reserve_blocks(xfs_mount_t *mp, uint64_t *inval,
 				xfs_fsop_resblks_t *outval);
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 3fbd98f61ea5..a62520f49ec5 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -2260,7 +2260,7 @@ xfs_file_ioctl(
 	}
 
 	case XFS_IOC_FSGROWFSDATA: {
-		xfs_growfs_data_t in;
+		struct xfs_growfs_data in;
 
 		if (copy_from_user(&in, arg, sizeof(in)))
 			return -EFAULT;
@@ -2274,7 +2274,7 @@ xfs_file_ioctl(
 	}
 
 	case XFS_IOC_FSGROWFSLOG: {
-		xfs_growfs_log_t in;
+		struct xfs_growfs_log in;
 
 		if (copy_from_user(&in, arg, sizeof(in)))
 			return -EFAULT;
-- 
2.27.0

Re: [PATCH v3 2/4] xfs: get rid of xfs_growfs_{data,log}_t

[Darrick J. Wong]

On Sat, Jan 09, 2021 at 03:09:17AM +0800, Gao Xiang wrote:
> Such usage isn't encouraged by the kernel coding style.
> 
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_fs.h |  4 ++--
>  fs/xfs/xfs_fsops.c     | 12 ++++++------
>  fs/xfs/xfs_fsops.h     |  4 ++--
>  fs/xfs/xfs_ioctl.c     |  4 ++--
>  4 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> index 2a2e3cfd94f0..a17313efc1fe 100644
> --- a/fs/xfs/libxfs/xfs_fs.h
> +++ b/fs/xfs/libxfs/xfs_fs.h
> @@ -308,12 +308,12 @@ struct xfs_ag_geometry {
>  typedef struct xfs_growfs_data {
>  	__u64		newblocks;	/* new data subvol size, fsblocks */
>  	__u32		imaxpct;	/* new inode space percentage limit */
> -} xfs_growfs_data_t;
> +};

So long as Eric is ok with fixing this up in xfs_fs_compat.h in
userspace,

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D


>  
>  typedef struct xfs_growfs_log {
>  	__u32		newblocks;	/* new log size, fsblocks */
>  	__u32		isint;		/* 1 if new log is internal */
> -} xfs_growfs_log_t;
> +};
>  
>  typedef struct xfs_growfs_rt {
>  	__u64		newblocks;	/* new realtime size, fsblocks */
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index d254588f6e21..6c5f6a50da2e 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/fs/xfs/xfs_fsops.c
> @@ -25,8 +25,8 @@
>   */
>  static int
>  xfs_growfs_data_private(
> -	xfs_mount_t		*mp,		/* mount point for filesystem */
> -	xfs_growfs_data_t	*in)		/* growfs data input struct */
> +	struct xfs_mount	*mp,		/* mount point for filesystem */
> +	struct xfs_growfs_data	*in)		/* growfs data input struct */
>  {
>  	struct xfs_buf		*bp;
>  	int			error;
> @@ -35,7 +35,7 @@ xfs_growfs_data_private(
>  	xfs_rfsblock_t		nb, nb_mod;
>  	xfs_rfsblock_t		delta;
>  	xfs_agnumber_t		oagcount;
> -	xfs_trans_t		*tp;
> +	struct xfs_trans	*tp;
>  	struct aghdr_init_data	id = {};
>  
>  	nb = in->newblocks;
> @@ -170,8 +170,8 @@ xfs_growfs_data_private(
>  
>  static int
>  xfs_growfs_log_private(
> -	xfs_mount_t		*mp,	/* mount point for filesystem */
> -	xfs_growfs_log_t	*in)	/* growfs log input struct */
> +	struct xfs_mount	*mp,	/* mount point for filesystem */
> +	struct xfs_growfs_log	*in)	/* growfs log input struct */
>  {
>  	xfs_extlen_t		nb;
>  
> @@ -268,7 +268,7 @@ xfs_growfs_data(
>  int
>  xfs_growfs_log(
>  	xfs_mount_t		*mp,
> -	xfs_growfs_log_t	*in)
> +	struct xfs_growfs_log	*in)
>  {
>  	int error;
>  
> diff --git a/fs/xfs/xfs_fsops.h b/fs/xfs/xfs_fsops.h
> index 92869f6ec8d3..d7e9af4a28eb 100644
> --- a/fs/xfs/xfs_fsops.h
> +++ b/fs/xfs/xfs_fsops.h
> @@ -6,8 +6,8 @@
>  #ifndef __XFS_FSOPS_H__
>  #define	__XFS_FSOPS_H__
>  
> -extern int xfs_growfs_data(xfs_mount_t *mp, xfs_growfs_data_t *in);
> -extern int xfs_growfs_log(xfs_mount_t *mp, xfs_growfs_log_t *in);
> +extern int xfs_growfs_data(xfs_mount_t *mp, struct xfs_growfs_data *in);
> +extern int xfs_growfs_log(xfs_mount_t *mp, struct xfs_growfs_log *in);
>  extern void xfs_fs_counts(xfs_mount_t *mp, xfs_fsop_counts_t *cnt);
>  extern int xfs_reserve_blocks(xfs_mount_t *mp, uint64_t *inval,
>  				xfs_fsop_resblks_t *outval);
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 3fbd98f61ea5..a62520f49ec5 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -2260,7 +2260,7 @@ xfs_file_ioctl(
>  	}
>  
>  	case XFS_IOC_FSGROWFSDATA: {
> -		xfs_growfs_data_t in;
> +		struct xfs_growfs_data in;
>  
>  		if (copy_from_user(&in, arg, sizeof(in)))
>  			return -EFAULT;
> @@ -2274,7 +2274,7 @@ xfs_file_ioctl(
>  	}
>  
>  	case XFS_IOC_FSGROWFSLOG: {
> -		xfs_growfs_log_t in;
> +		struct xfs_growfs_log in;
>  
>  		if (copy_from_user(&in, arg, sizeof(in)))
>  			return -EFAULT;
> -- 
> 2.27.0
> 

Re: [PATCH v3 2/4] xfs: get rid of xfs_growfs_{data,log}_t

[Eric Sandeen]

On 1/8/21 3:21 PM, Darrick J. Wong wrote:
> On Sat, Jan 09, 2021 at 03:09:17AM +0800, Gao Xiang wrote:
>> Such usage isn't encouraged by the kernel coding style.
>>
>> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
>> ---
>>  fs/xfs/libxfs/xfs_fs.h |  4 ++--
>>  fs/xfs/xfs_fsops.c     | 12 ++++++------
>>  fs/xfs/xfs_fsops.h     |  4 ++--
>>  fs/xfs/xfs_ioctl.c     |  4 ++--
>>  4 files changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
>> index 2a2e3cfd94f0..a17313efc1fe 100644
>> --- a/fs/xfs/libxfs/xfs_fs.h
>> +++ b/fs/xfs/libxfs/xfs_fs.h
>> @@ -308,12 +308,12 @@ struct xfs_ag_geometry {
>>  typedef struct xfs_growfs_data {
>>  	__u64		newblocks;	/* new data subvol size, fsblocks */
>>  	__u32		imaxpct;	/* new inode space percentage limit */
>> -} xfs_growfs_data_t;
>> +};
> 
> So long as Eric is ok with fixing this up in xfs_fs_compat.h in
> userspace,
> 
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

Sure, why not :) (tho is growfs really a public interface?  I guess so,
technically, though not documented as such.)

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

-Eric

Re: [PATCH v3 2/4] xfs: get rid of xfs_growfs_{data,log}_t

[Gao Xiang]

On Fri, Jan 08, 2021 at 03:27:21PM -0600, Eric Sandeen wrote:
> On 1/8/21 3:21 PM, Darrick J. Wong wrote:
> > On Sat, Jan 09, 2021 at 03:09:17AM +0800, Gao Xiang wrote:
> >> Such usage isn't encouraged by the kernel coding style.
> >>
> >> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> >> ---
> >>  fs/xfs/libxfs/xfs_fs.h |  4 ++--
> >>  fs/xfs/xfs_fsops.c     | 12 ++++++------
> >>  fs/xfs/xfs_fsops.h     |  4 ++--
> >>  fs/xfs/xfs_ioctl.c     |  4 ++--
> >>  4 files changed, 12 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> >> index 2a2e3cfd94f0..a17313efc1fe 100644
> >> --- a/fs/xfs/libxfs/xfs_fs.h
> >> +++ b/fs/xfs/libxfs/xfs_fs.h
> >> @@ -308,12 +308,12 @@ struct xfs_ag_geometry {
> >>  typedef struct xfs_growfs_data {
> >>  	__u64		newblocks;	/* new data subvol size, fsblocks */
> >>  	__u32		imaxpct;	/* new inode space percentage limit */
> >> -} xfs_growfs_data_t;
> >> +};
> > 
> > So long as Eric is ok with fixing this up in xfs_fs_compat.h in
> > userspace,
> > 
> > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Sure, why not :) (tho is growfs really a public interface?  I guess so,
> technically, though not documented as such.)

Yeah, although I think nobody else uses it (I could leave the typedef
definitions only if needed otherwise...)

Thanks,
Gao Xiang

> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> 
> -Eric
> 

Re: [PATCH v3 2/4] xfs: get rid of xfs_growfs_{data,log}_t

[Dave Chinner]

On Sat, Jan 09, 2021 at 08:49:34AM +0800, Gao Xiang wrote:
> On Fri, Jan 08, 2021 at 03:27:21PM -0600, Eric Sandeen wrote:
> > On 1/8/21 3:21 PM, Darrick J. Wong wrote:
> > > On Sat, Jan 09, 2021 at 03:09:17AM +0800, Gao Xiang wrote:
> > >> Such usage isn't encouraged by the kernel coding style.
> > >>
> > >> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > >> ---
> > >>  fs/xfs/libxfs/xfs_fs.h |  4 ++--
> > >>  fs/xfs/xfs_fsops.c     | 12 ++++++------
> > >>  fs/xfs/xfs_fsops.h     |  4 ++--
> > >>  fs/xfs/xfs_ioctl.c     |  4 ++--
> > >>  4 files changed, 12 insertions(+), 12 deletions(-)
> > >>
> > >> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> > >> index 2a2e3cfd94f0..a17313efc1fe 100644
> > >> --- a/fs/xfs/libxfs/xfs_fs.h
> > >> +++ b/fs/xfs/libxfs/xfs_fs.h
> > >> @@ -308,12 +308,12 @@ struct xfs_ag_geometry {
> > >>  typedef struct xfs_growfs_data {
> > >>  	__u64		newblocks;	/* new data subvol size, fsblocks */
> > >>  	__u32		imaxpct;	/* new inode space percentage limit */
> > >> -} xfs_growfs_data_t;
> > >> +};
> > > 
> > > So long as Eric is ok with fixing this up in xfs_fs_compat.h in
> > > userspace,
> > > 
> > > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Sure, why not :) (tho is growfs really a public interface?  I guess so,
> > technically, though not documented as such.)

They are not described in man pages, though they are listed in
xfsctl(3) so they are definitely public interfaces.

> Yeah, although I think nobody else uses it (I could leave the typedef
> definitions only if needed otherwise...)

It is used elsewhere - ISTR that it is used by a couple of third
party applications that integrate growing filesystems into their
other storage management tasks.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v3 2/4] xfs: get rid of xfs_growfs_{data,log}_t

[Gao Xiang]

On Mon, Jan 11, 2021 at 08:04:36AM +1100, Dave Chinner wrote:
> On Sat, Jan 09, 2021 at 08:49:34AM +0800, Gao Xiang wrote:
> > On Fri, Jan 08, 2021 at 03:27:21PM -0600, Eric Sandeen wrote:
> > > On 1/8/21 3:21 PM, Darrick J. Wong wrote:
> > > > On Sat, Jan 09, 2021 at 03:09:17AM +0800, Gao Xiang wrote:
> > > >> Such usage isn't encouraged by the kernel coding style.
> > > >>
> > > >> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > > >> ---
> > > >>  fs/xfs/libxfs/xfs_fs.h |  4 ++--
> > > >>  fs/xfs/xfs_fsops.c     | 12 ++++++------
> > > >>  fs/xfs/xfs_fsops.h     |  4 ++--
> > > >>  fs/xfs/xfs_ioctl.c     |  4 ++--
> > > >>  4 files changed, 12 insertions(+), 12 deletions(-)
> > > >>
> > > >> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> > > >> index 2a2e3cfd94f0..a17313efc1fe 100644
> > > >> --- a/fs/xfs/libxfs/xfs_fs.h
> > > >> +++ b/fs/xfs/libxfs/xfs_fs.h
> > > >> @@ -308,12 +308,12 @@ struct xfs_ag_geometry {
> > > >>  typedef struct xfs_growfs_data {
> > > >>  	__u64		newblocks;	/* new data subvol size, fsblocks */
> > > >>  	__u32		imaxpct;	/* new inode space percentage limit */
> > > >> -} xfs_growfs_data_t;
> > > >> +};
> > > > 
> > > > So long as Eric is ok with fixing this up in xfs_fs_compat.h in
> > > > userspace,
> > > > 
> > > > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > Sure, why not :) (tho is growfs really a public interface?  I guess so,
> > > technically, though not documented as such.)
> 
> They are not described in man pages, though they are listed in
> xfsctl(3) so they are definitely public interfaces.
> 
> > Yeah, although I think nobody else uses it (I could leave the typedef
> > definitions only if needed otherwise...)
> 
> It is used elsewhere - ISTR that it is used by a couple of third
> party applications that integrate growing filesystems into their
> other storage management tasks.

Okay, will leave the definitions in the next version.

Thanks,
Gao Xiang

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 

Re: [PATCH v3 2/4] xfs: get rid of xfs_growfs_{data,log}_t

[Christoph Hellwig]

On Mon, Jan 11, 2021 at 08:17:49AM +0800, Gao Xiang wrote:
> Okay, will leave the definitions in the next version.

Note that the important thing is to not break the kernel to userspace
ABI, the API is a little less important.  That being said breaking
programs piecemail for no good reason't isn't nice.  I think we
should probably move all the typedefs to the end of the file with
a comment and then at some point remove them as a single breaking
change.

Re: [PATCH v3 2/4] xfs: get rid of xfs_growfs_{data,log}_t

[Gao Xiang]

On Mon, Jan 11, 2021 at 05:30:54PM +0000, Christoph Hellwig wrote:
> On Mon, Jan 11, 2021 at 08:17:49AM +0800, Gao Xiang wrote:
> > Okay, will leave the definitions in the next version.
> 
> Note that the important thing is to not break the kernel to userspace
> ABI, the API is a little less important.  That being said breaking
> programs piecemail for no good reason't isn't nice.  I think we
> should probably move all the typedefs to the end of the file with
> a comment and then at some point remove them as a single breaking
> change.

Agree with your point. Yet I'm not sure if it's much related to this
series (I raised this due to touch some code)... I think it'd be better
as an independent xfs-treewide patch to move all userspace visable
typedefs to a unique place at once (not just these two typedefs...)
But I'm also ok if just move these two types in advance if needed.

(I suffer from lack of modification of old typedefs out of copy &
 paste from time to time. IMHO, I hope such exist typedef usage
 could be removed as soon as possible... )

Thanks,
Gao Xiang

> 

[PATCH v3 3/4] xfs: hoist out xfs_resizefs_init_new_ags()

[Gao Xiang]

Move out related logic for initializing new added AGs to a new helper
in preparation for shrinking. No logic changes.

Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
 fs/xfs/xfs_fsops.c | 74 +++++++++++++++++++++++++++-------------------
 1 file changed, 44 insertions(+), 30 deletions(-)

diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index 6c5f6a50da2e..a792d1f0ac55 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -20,6 +20,49 @@
 #include "xfs_ag.h"
 #include "xfs_ag_resv.h"
 
+static int
+xfs_resizefs_init_new_ags(
+	xfs_mount_t		*mp,
+	struct aghdr_init_data	*id,
+	xfs_agnumber_t		oagcount,
+	xfs_agnumber_t		nagcount,
+	xfs_rfsblock_t		*delta)
+{
+	xfs_rfsblock_t		nb = mp->m_sb.sb_dblocks + *delta;
+	int			error;
+
+	/*
+	 * Write new AG headers to disk. Non-transactional, but need to be
+	 * written and completed prior to the growfs transaction being logged.
+	 * To do this, we use a delayed write buffer list and wait for
+	 * submission and IO completion of the list as a whole. This allows the
+	 * IO subsystem to merge all the AG headers in a single AG into a single
+	 * IO and hide most of the latency of the IO from us.
+	 *
+	 * This also means that if we get an error whilst building the buffer
+	 * list to write, we can cancel the entire list without having written
+	 * anything.
+	 */
+	INIT_LIST_HEAD(&id->buffer_list);
+	for (id->agno = nagcount - 1;
+	     id->agno >= oagcount;
+	     id->agno--, *delta -= id->agsize) {
+
+		if (id->agno == nagcount - 1)
+			id->agsize = nb - (id->agno *
+					(xfs_rfsblock_t)mp->m_sb.sb_agblocks);
+		else
+			id->agsize = mp->m_sb.sb_agblocks;
+
+		error = xfs_ag_init_headers(mp, id);
+		if (error) {
+			xfs_buf_delwri_cancel(&id->buffer_list);
+			return error;
+		}
+	}
+	return xfs_buf_delwri_submit(&id->buffer_list);
+}
+
 /*
  * growfs operations
  */
@@ -74,36 +117,7 @@ xfs_growfs_data_private(
 	if (error)
 		return error;
 
-	/*
-	 * Write new AG headers to disk. Non-transactional, but need to be
-	 * written and completed prior to the growfs transaction being logged.
-	 * To do this, we use a delayed write buffer list and wait for
-	 * submission and IO completion of the list as a whole. This allows the
-	 * IO subsystem to merge all the AG headers in a single AG into a single
-	 * IO and hide most of the latency of the IO from us.
-	 *
-	 * This also means that if we get an error whilst building the buffer
-	 * list to write, we can cancel the entire list without having written
-	 * anything.
-	 */
-	INIT_LIST_HEAD(&id.buffer_list);
-	for (id.agno = nagcount - 1;
-	     id.agno >= oagcount;
-	     id.agno--, delta -= id.agsize) {
-
-		if (id.agno == nagcount - 1)
-			id.agsize = nb -
-				(id.agno * (xfs_rfsblock_t)mp->m_sb.sb_agblocks);
-		else
-			id.agsize = mp->m_sb.sb_agblocks;
-
-		error = xfs_ag_init_headers(mp, &id);
-		if (error) {
-			xfs_buf_delwri_cancel(&id.buffer_list);
-			goto out_trans_cancel;
-		}
-	}
-	error = xfs_buf_delwri_submit(&id.buffer_list);
+	error = xfs_resizefs_init_new_ags(mp, &id, oagcount, nagcount, &delta);
 	if (error)
 		goto out_trans_cancel;
 
-- 
2.27.0

Re: [PATCH v3 3/4] xfs: hoist out xfs_resizefs_init_new_ags()

[Darrick J. Wong]

On Sat, Jan 09, 2021 at 03:09:18AM +0800, Gao Xiang wrote:
> Move out related logic for initializing new added AGs to a new helper
> in preparation for shrinking. No logic changes.
> 
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
>  fs/xfs/xfs_fsops.c | 74 +++++++++++++++++++++++++++-------------------

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

>  1 file changed, 44 insertions(+), 30 deletions(-)
> 
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index 6c5f6a50da2e..a792d1f0ac55 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/fs/xfs/xfs_fsops.c
> @@ -20,6 +20,49 @@
>  #include "xfs_ag.h"
>  #include "xfs_ag_resv.h"
>  
> +static int
> +xfs_resizefs_init_new_ags(
> +	xfs_mount_t		*mp,
> +	struct aghdr_init_data	*id,
> +	xfs_agnumber_t		oagcount,
> +	xfs_agnumber_t		nagcount,
> +	xfs_rfsblock_t		*delta)
> +{
> +	xfs_rfsblock_t		nb = mp->m_sb.sb_dblocks + *delta;
> +	int			error;
> +
> +	/*
> +	 * Write new AG headers to disk. Non-transactional, but need to be
> +	 * written and completed prior to the growfs transaction being logged.
> +	 * To do this, we use a delayed write buffer list and wait for
> +	 * submission and IO completion of the list as a whole. This allows the
> +	 * IO subsystem to merge all the AG headers in a single AG into a single
> +	 * IO and hide most of the latency of the IO from us.
> +	 *
> +	 * This also means that if we get an error whilst building the buffer
> +	 * list to write, we can cancel the entire list without having written
> +	 * anything.
> +	 */
> +	INIT_LIST_HEAD(&id->buffer_list);
> +	for (id->agno = nagcount - 1;
> +	     id->agno >= oagcount;
> +	     id->agno--, *delta -= id->agsize) {
> +
> +		if (id->agno == nagcount - 1)
> +			id->agsize = nb - (id->agno *
> +					(xfs_rfsblock_t)mp->m_sb.sb_agblocks);
> +		else
> +			id->agsize = mp->m_sb.sb_agblocks;
> +
> +		error = xfs_ag_init_headers(mp, id);
> +		if (error) {
> +			xfs_buf_delwri_cancel(&id->buffer_list);
> +			return error;
> +		}
> +	}
> +	return xfs_buf_delwri_submit(&id->buffer_list);
> +}
> +
>  /*
>   * growfs operations
>   */
> @@ -74,36 +117,7 @@ xfs_growfs_data_private(
>  	if (error)
>  		return error;
>  
> -	/*
> -	 * Write new AG headers to disk. Non-transactional, but need to be
> -	 * written and completed prior to the growfs transaction being logged.
> -	 * To do this, we use a delayed write buffer list and wait for
> -	 * submission and IO completion of the list as a whole. This allows the
> -	 * IO subsystem to merge all the AG headers in a single AG into a single
> -	 * IO and hide most of the latency of the IO from us.
> -	 *
> -	 * This also means that if we get an error whilst building the buffer
> -	 * list to write, we can cancel the entire list without having written
> -	 * anything.
> -	 */
> -	INIT_LIST_HEAD(&id.buffer_list);
> -	for (id.agno = nagcount - 1;
> -	     id.agno >= oagcount;
> -	     id.agno--, delta -= id.agsize) {
> -
> -		if (id.agno == nagcount - 1)
> -			id.agsize = nb -
> -				(id.agno * (xfs_rfsblock_t)mp->m_sb.sb_agblocks);
> -		else
> -			id.agsize = mp->m_sb.sb_agblocks;
> -
> -		error = xfs_ag_init_headers(mp, &id);
> -		if (error) {
> -			xfs_buf_delwri_cancel(&id.buffer_list);
> -			goto out_trans_cancel;
> -		}
> -	}
> -	error = xfs_buf_delwri_submit(&id.buffer_list);
> +	error = xfs_resizefs_init_new_ags(mp, &id, oagcount, nagcount, &delta);
>  	if (error)
>  		goto out_trans_cancel;
>  
> -- 
> 2.27.0
> 

Re: [PATCH v3 3/4] xfs: hoist out xfs_resizefs_init_new_ags()

[Christoph Hellwig]

> +	xfs_mount_t		*mp,

Please use the struct type here.

> +	/*
> +	 * Write new AG headers to disk. Non-transactional, but need to be
> +	 * written and completed prior to the growfs transaction being logged.
> +	 * To do this, we use a delayed write buffer list and wait for
> +	 * submission and IO completion of the list as a whole. This allows the
> +	 * IO subsystem to merge all the AG headers in a single AG into a single
> +	 * IO and hide most of the latency of the IO from us.
> +	 *
> +	 * This also means that if we get an error whilst building the buffer
> +	 * list to write, we can cancel the entire list without having written
> +	 * anything.
> +	 */

Maybe move the comment on top of the whole function, as it really
explains what the function does.

Re: [PATCH v3 3/4] xfs: hoist out xfs_resizefs_init_new_ags()

[Gao Xiang]

On Mon, Jan 11, 2021 at 05:32:49PM +0000, Christoph Hellwig wrote:
> > +	xfs_mount_t		*mp,
> 
> Please use the struct type here.

Sigh... sometimes still forget to modify after copy & paste from
somewhere... Will fix tomorrow.

> 
> > +	/*
> > +	 * Write new AG headers to disk. Non-transactional, but need to be
> > +	 * written and completed prior to the growfs transaction being logged.
> > +	 * To do this, we use a delayed write buffer list and wait for
> > +	 * submission and IO completion of the list as a whole. This allows the
> > +	 * IO subsystem to merge all the AG headers in a single AG into a single
> > +	 * IO and hide most of the latency of the IO from us.
> > +	 *
> > +	 * This also means that if we get an error whilst building the buffer
> > +	 * list to write, we can cancel the entire list without having written
> > +	 * anything.
> > +	 */
> 
> Maybe move the comment on top of the whole function, as it really
> explains what the function does.

Ok. Will update tomorrow too.

Thanks,
Gao Xiang

> 

[PATCH v3 4/4] xfs: support shrinking unused space in the last AG

[Gao Xiang]

As the first step of shrinking, this attempts to enable shrinking
unused space in the last allocation group by fixing up freespace
btree, agi, agf and adjusting super block and introduce a helper
xfs_ag_shrink_space() to fixup the last AG.

This can be all done in one transaction for now, so I think no
additional protection is needed.

Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
 fs/xfs/libxfs/xfs_ag.c | 72 ++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/libxfs/xfs_ag.h |  2 ++
 fs/xfs/xfs_fsops.c     | 69 ++++++++++++++++++++++++++++++----------
 fs/xfs/xfs_trans.c     |  1 -
 4 files changed, 126 insertions(+), 18 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
index 9331f3516afa..bec10c85e2a9 100644
--- a/fs/xfs/libxfs/xfs_ag.c
+++ b/fs/xfs/libxfs/xfs_ag.c
@@ -485,6 +485,78 @@ xfs_ag_init_headers(
 	return error;
 }
 
+int
+xfs_ag_shrink_space(
+	struct xfs_mount	*mp,
+	struct xfs_trans	*tp,
+	struct aghdr_init_data	*id,
+	xfs_extlen_t		len)
+{
+	struct xfs_buf		*agibp, *agfbp;
+	struct xfs_agi		*agi;
+	struct xfs_agf		*agf;
+	int			error, err2;
+	struct xfs_alloc_arg	args = {
+		.tp	= tp,
+		.mp	= mp,
+		.type	= XFS_ALLOCTYPE_THIS_BNO,
+		.minlen = len,
+		.maxlen = len,
+		.oinfo	= XFS_RMAP_OINFO_SKIP_UPDATE,
+		.resv	= XFS_AG_RESV_NONE,
+		.prod	= 1
+	};
+
+	ASSERT(id->agno == mp->m_sb.sb_agcount - 1);
+	error = xfs_ialloc_read_agi(mp, tp, id->agno, &agibp);
+	if (error)
+		return error;
+
+	agi = agibp->b_addr;
+
+	error = xfs_alloc_read_agf(mp, tp, id->agno, 0, &agfbp);
+	if (error)
+		return error;
+
+	args.fsbno = XFS_AGB_TO_FSB(mp, id->agno,
+				    be32_to_cpu(agi->agi_length) - len);
+
+	/* remove the preallocations before allocation and re-establish then */
+	error = xfs_ag_resv_free(agibp->b_pag);
+	if (error)
+		return error;
+
+	/* internal log shouldn't also show up in the free space btrees */
+	error = xfs_alloc_vextent(&args);
+	if (error)
+		goto out;
+
+	if (args.agbno == NULLAGBLOCK) {
+		error = -ENOSPC;
+		goto out;
+	}
+
+	/* Change the agi length */
+	be32_add_cpu(&agi->agi_length, -len);
+	xfs_ialloc_log_agi(tp, agibp, XFS_AGI_LENGTH);
+
+	/* Change agf length */
+	agf = agfbp->b_addr;
+	be32_add_cpu(&agf->agf_length, -len);
+	ASSERT(agf->agf_length == agi->agi_length);
+	xfs_alloc_log_agf(tp, agfbp, XFS_AGF_LENGTH);
+
+out:
+	err2 = xfs_ag_resv_init(agibp->b_pag, tp);
+	if (err2 && err2 != -ENOSPC) {
+		xfs_warn(mp,
+"Error %d reserving per-AG metadata reserve pool.", err2);
+		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
+		return err2;
+	}
+	return error;
+}
+
 /*
  * Extent the AG indicated by the @id by the length passed in
  */
diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h
index 5166322807e7..f3b5bbfeadce 100644
--- a/fs/xfs/libxfs/xfs_ag.h
+++ b/fs/xfs/libxfs/xfs_ag.h
@@ -24,6 +24,8 @@ struct aghdr_init_data {
 };
 
 int xfs_ag_init_headers(struct xfs_mount *mp, struct aghdr_init_data *id);
+int xfs_ag_shrink_space(struct xfs_mount *mp, struct xfs_trans *tp,
+			struct aghdr_init_data *id, xfs_extlen_t len);
 int xfs_ag_extend_space(struct xfs_mount *mp, struct xfs_trans *tp,
 			struct aghdr_init_data *id, xfs_extlen_t len);
 int xfs_ag_get_geometry(struct xfs_mount *mp, xfs_agnumber_t agno,
diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index a792d1f0ac55..eea38395e804 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -79,19 +79,22 @@ xfs_growfs_data_private(
 	xfs_rfsblock_t		delta;
 	xfs_agnumber_t		oagcount;
 	struct xfs_trans	*tp;
+	bool			extend;
 	struct aghdr_init_data	id = {};
 
 	nb = in->newblocks;
-	if (nb < mp->m_sb.sb_dblocks)
-		return -EINVAL;
-	if ((error = xfs_sb_validate_fsb_count(&mp->m_sb, nb)))
+	error = xfs_sb_validate_fsb_count(&mp->m_sb, nb);
+	if (error)
 		return error;
-	error = xfs_buf_read_uncached(mp->m_ddev_targp,
+
+	if (nb > mp->m_sb.sb_dblocks) {
+		error = xfs_buf_read_uncached(mp->m_ddev_targp,
 				XFS_FSB_TO_BB(mp, nb) - XFS_FSS_TO_BB(mp, 1),
 				XFS_FSS_TO_BB(mp, 1), 0, &bp, NULL);
-	if (error)
-		return error;
-	xfs_buf_relse(bp);
+		if (error)
+			return error;
+		xfs_buf_relse(bp);
+	}
 
 	delta = nb;	/* use delta as a temporary here */
 	nb_mod = do_div(delta, mp->m_sb.sb_agblocks);
@@ -99,10 +102,18 @@ xfs_growfs_data_private(
 	if (nb_mod && nb_mod < XFS_MIN_AG_BLOCKS) {
 		nagcount--;
 		nb = (xfs_rfsblock_t)nagcount * mp->m_sb.sb_agblocks;
-		if (nb < mp->m_sb.sb_dblocks)
+		if (nagcount < 2)
 			return -EINVAL;
 	}
-	delta = nb - mp->m_sb.sb_dblocks;
+
+	if (nb > mp->m_sb.sb_dblocks) {
+		delta = nb - mp->m_sb.sb_dblocks;
+		extend = true;
+	} else {
+		delta = mp->m_sb.sb_dblocks - nb;
+		extend = false;
+	}
+
 	oagcount = mp->m_sb.sb_agcount;
 
 	/* allocate the new per-ag structures */
@@ -110,22 +121,34 @@ xfs_growfs_data_private(
 		error = xfs_initialize_perag(mp, nagcount, &nagimax);
 		if (error)
 			return error;
+	} else if (nagcount != oagcount) {
+		/* TODO: shrinking the entire AGs hasn't yet completed */
+		return -EINVAL;
 	}
 
 	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_growdata,
-			XFS_GROWFS_SPACE_RES(mp), 0, XFS_TRANS_RESERVE, &tp);
+			(extend ? XFS_GROWFS_SPACE_RES(mp) : delta), 0,
+			XFS_TRANS_RESERVE, &tp);
 	if (error)
 		return error;
 
-	error = xfs_resizefs_init_new_ags(mp, &id, oagcount, nagcount, &delta);
-	if (error)
-		goto out_trans_cancel;
-
+	if (extend) {
+		error = xfs_resizefs_init_new_ags(mp, &id, oagcount,
+						  nagcount, &delta);
+		if (error)
+			goto out_trans_cancel;
+	}
 	xfs_trans_agblocks_delta(tp, id.nfree);
 
-	/* If there are new blocks in the old last AG, extend it. */
+	/* If there are some blocks in the last AG, resize it. */
 	if (delta) {
-		error = xfs_ag_extend_space(mp, tp, &id, delta);
+		if (extend) {
+			error = xfs_ag_extend_space(mp, tp, &id, delta);
+		} else {
+			id.agno = nagcount - 1;
+			error = xfs_ag_shrink_space(mp, tp, &id, delta);
+		}
+
 		if (error)
 			goto out_trans_cancel;
 	}
@@ -137,11 +160,19 @@ xfs_growfs_data_private(
 	 */
 	if (nagcount > oagcount)
 		xfs_trans_mod_sb(tp, XFS_TRANS_SB_AGCOUNT, nagcount - oagcount);
-	if (nb > mp->m_sb.sb_dblocks)
+	if (nb != mp->m_sb.sb_dblocks)
 		xfs_trans_mod_sb(tp, XFS_TRANS_SB_DBLOCKS,
 				 nb - mp->m_sb.sb_dblocks);
 	if (id.nfree)
 		xfs_trans_mod_sb(tp, XFS_TRANS_SB_FDBLOCKS, id.nfree);
+
+	/*
+	 * update in-core counters (especially sb_fdblocks) now
+	 * so xfs_validate_sb_write() can pass.
+	 */
+	if (xfs_sb_version_haslazysbcount(&mp->m_sb))
+		xfs_log_sb(tp);
+
 	xfs_trans_set_sync(tp);
 	error = xfs_trans_commit(tp);
 	if (error)
@@ -178,6 +209,10 @@ xfs_growfs_data_private(
 	return error;
 
 out_trans_cancel:
+	if (!extend && (tp->t_flags & XFS_TRANS_DIRTY)) {
+		xfs_trans_commit(tp);
+		return error;
+	}
 	xfs_trans_cancel(tp);
 	return error;
 }
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index e72730f85af1..fd2cbf414b80 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -419,7 +419,6 @@ xfs_trans_mod_sb(
 		tp->t_res_frextents_delta += delta;
 		break;
 	case XFS_TRANS_SB_DBLOCKS:
-		ASSERT(delta > 0);
 		tp->t_dblocks_delta += delta;
 		break;
 	case XFS_TRANS_SB_AGCOUNT:
-- 
2.27.0

Re: [PATCH v3 4/4] xfs: support shrinking unused space in the last AG

[Darrick J. Wong]

On Sat, Jan 09, 2021 at 03:09:19AM +0800, Gao Xiang wrote:
> As the first step of shrinking, this attempts to enable shrinking
> unused space in the last allocation group by fixing up freespace
> btree, agi, agf and adjusting super block and introduce a helper
> xfs_ag_shrink_space() to fixup the last AG.
> 
> This can be all done in one transaction for now, so I think no
> additional protection is needed.
> 
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_ag.c | 72 ++++++++++++++++++++++++++++++++++++++++++
>  fs/xfs/libxfs/xfs_ag.h |  2 ++
>  fs/xfs/xfs_fsops.c     | 69 ++++++++++++++++++++++++++++++----------
>  fs/xfs/xfs_trans.c     |  1 -
>  4 files changed, 126 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
> index 9331f3516afa..bec10c85e2a9 100644
> --- a/fs/xfs/libxfs/xfs_ag.c
> +++ b/fs/xfs/libxfs/xfs_ag.c
> @@ -485,6 +485,78 @@ xfs_ag_init_headers(
>  	return error;
>  }
>  
> +int
> +xfs_ag_shrink_space(
> +	struct xfs_mount	*mp,
> +	struct xfs_trans	*tp,
> +	struct aghdr_init_data	*id,
> +	xfs_extlen_t		len)
> +{
> +	struct xfs_buf		*agibp, *agfbp;
> +	struct xfs_agi		*agi;
> +	struct xfs_agf		*agf;
> +	int			error, err2;
> +	struct xfs_alloc_arg	args = {
> +		.tp	= tp,
> +		.mp	= mp,
> +		.type	= XFS_ALLOCTYPE_THIS_BNO,
> +		.minlen = len,
> +		.maxlen = len,
> +		.oinfo	= XFS_RMAP_OINFO_SKIP_UPDATE,
> +		.resv	= XFS_AG_RESV_NONE,
> +		.prod	= 1
> +	};
> +
> +	ASSERT(id->agno == mp->m_sb.sb_agcount - 1);
> +	error = xfs_ialloc_read_agi(mp, tp, id->agno, &agibp);
> +	if (error)
> +		return error;
> +
> +	agi = agibp->b_addr;
> +
> +	error = xfs_alloc_read_agf(mp, tp, id->agno, 0, &agfbp);
> +	if (error)
> +		return error;
> +
> +	args.fsbno = XFS_AGB_TO_FSB(mp, id->agno,
> +				    be32_to_cpu(agi->agi_length) - len);
> +
> +	/* remove the preallocations before allocation and re-establish then */
> +	error = xfs_ag_resv_free(agibp->b_pag);
> +	if (error)
> +		return error;
> +
> +	/* internal log shouldn't also show up in the free space btrees */
> +	error = xfs_alloc_vextent(&args);
> +	if (error)
> +		goto out;
> +
> +	if (args.agbno == NULLAGBLOCK) {
> +		error = -ENOSPC;
> +		goto out;
> +	}
> +
> +	/* Change the agi length */
> +	be32_add_cpu(&agi->agi_length, -len);
> +	xfs_ialloc_log_agi(tp, agibp, XFS_AGI_LENGTH);
> +
> +	/* Change agf length */
> +	agf = agfbp->b_addr;
> +	be32_add_cpu(&agf->agf_length, -len);
> +	ASSERT(agf->agf_length == agi->agi_length);
> +	xfs_alloc_log_agf(tp, agfbp, XFS_AGF_LENGTH);
> +
> +out:
> +	err2 = xfs_ag_resv_init(agibp->b_pag, tp);
> +	if (err2 && err2 != -ENOSPC) {
> +		xfs_warn(mp,
> +"Error %d reserving per-AG metadata reserve pool.", err2);
> +		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> +		return err2;
> +	}
> +	return error;
> +}
> +
>  /*
>   * Extent the AG indicated by the @id by the length passed in
>   */
> diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h
> index 5166322807e7..f3b5bbfeadce 100644
> --- a/fs/xfs/libxfs/xfs_ag.h
> +++ b/fs/xfs/libxfs/xfs_ag.h
> @@ -24,6 +24,8 @@ struct aghdr_init_data {
>  };
>  
>  int xfs_ag_init_headers(struct xfs_mount *mp, struct aghdr_init_data *id);
> +int xfs_ag_shrink_space(struct xfs_mount *mp, struct xfs_trans *tp,
> +			struct aghdr_init_data *id, xfs_extlen_t len);
>  int xfs_ag_extend_space(struct xfs_mount *mp, struct xfs_trans *tp,
>  			struct aghdr_init_data *id, xfs_extlen_t len);
>  int xfs_ag_get_geometry(struct xfs_mount *mp, xfs_agnumber_t agno,
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index a792d1f0ac55..eea38395e804 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/fs/xfs/xfs_fsops.c
> @@ -79,19 +79,22 @@ xfs_growfs_data_private(
>  	xfs_rfsblock_t		delta;
>  	xfs_agnumber_t		oagcount;
>  	struct xfs_trans	*tp;
> +	bool			extend;
>  	struct aghdr_init_data	id = {};
>  
>  	nb = in->newblocks;
> -	if (nb < mp->m_sb.sb_dblocks)
> -		return -EINVAL;
> -	if ((error = xfs_sb_validate_fsb_count(&mp->m_sb, nb)))
> +	error = xfs_sb_validate_fsb_count(&mp->m_sb, nb);
> +	if (error)
>  		return error;
> -	error = xfs_buf_read_uncached(mp->m_ddev_targp,
> +
> +	if (nb > mp->m_sb.sb_dblocks) {
> +		error = xfs_buf_read_uncached(mp->m_ddev_targp,
>  				XFS_FSB_TO_BB(mp, nb) - XFS_FSS_TO_BB(mp, 1),
>  				XFS_FSS_TO_BB(mp, 1), 0, &bp, NULL);
> -	if (error)
> -		return error;
> -	xfs_buf_relse(bp);
> +		if (error)
> +			return error;
> +		xfs_buf_relse(bp);
> +	}
>  
>  	delta = nb;	/* use delta as a temporary here */

Yikes, can this become a separate variable please?

>  	nb_mod = do_div(delta, mp->m_sb.sb_agblocks);
> @@ -99,10 +102,18 @@ xfs_growfs_data_private(
>  	if (nb_mod && nb_mod < XFS_MIN_AG_BLOCKS) {
>  		nagcount--;
>  		nb = (xfs_rfsblock_t)nagcount * mp->m_sb.sb_agblocks;
> -		if (nb < mp->m_sb.sb_dblocks)
> +		if (nagcount < 2)
>  			return -EINVAL;
>  	}
> -	delta = nb - mp->m_sb.sb_dblocks;
> +
> +	if (nb > mp->m_sb.sb_dblocks) {
> +		delta = nb - mp->m_sb.sb_dblocks;
> +		extend = true;
> +	} else {
> +		delta = mp->m_sb.sb_dblocks - nb;
> +		extend = false;

/me wonders why delta isn't simply int64_t, and then you can do things
like:

if (delta > 0)
	growfs
else if (delta < 0)
	shrinkfs

?

> +	}
> +
>  	oagcount = mp->m_sb.sb_agcount;
>  
>  	/* allocate the new per-ag structures */
> @@ -110,22 +121,34 @@ xfs_growfs_data_private(
>  		error = xfs_initialize_perag(mp, nagcount, &nagimax);
>  		if (error)
>  			return error;
> +	} else if (nagcount != oagcount) {
> +		/* TODO: shrinking the entire AGs hasn't yet completed */
> +		return -EINVAL;
>  	}
>  
>  	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_growdata,
> -			XFS_GROWFS_SPACE_RES(mp), 0, XFS_TRANS_RESERVE, &tp);
> +			(extend ? XFS_GROWFS_SPACE_RES(mp) : delta), 0,
> +			XFS_TRANS_RESERVE, &tp);
>  	if (error)
>  		return error;
>  
> -	error = xfs_resizefs_init_new_ags(mp, &id, oagcount, nagcount, &delta);
> -	if (error)
> -		goto out_trans_cancel;
> -
> +	if (extend) {
> +		error = xfs_resizefs_init_new_ags(mp, &id, oagcount,
> +						  nagcount, &delta);
> +		if (error)
> +			goto out_trans_cancel;
> +	}
>  	xfs_trans_agblocks_delta(tp, id.nfree);
>  
> -	/* If there are new blocks in the old last AG, extend it. */
> +	/* If there are some blocks in the last AG, resize it. */
>  	if (delta) {
> -		error = xfs_ag_extend_space(mp, tp, &id, delta);
> +		if (extend) {
> +			error = xfs_ag_extend_space(mp, tp, &id, delta);
> +		} else {
> +			id.agno = nagcount - 1;
> +			error = xfs_ag_shrink_space(mp, tp, &id, delta);
> +		}
> +
>  		if (error)
>  			goto out_trans_cancel;
>  	}
> @@ -137,11 +160,19 @@ xfs_growfs_data_private(
>  	 */
>  	if (nagcount > oagcount)
>  		xfs_trans_mod_sb(tp, XFS_TRANS_SB_AGCOUNT, nagcount - oagcount);
> -	if (nb > mp->m_sb.sb_dblocks)
> +	if (nb != mp->m_sb.sb_dblocks)
>  		xfs_trans_mod_sb(tp, XFS_TRANS_SB_DBLOCKS,
>  				 nb - mp->m_sb.sb_dblocks);
>  	if (id.nfree)
>  		xfs_trans_mod_sb(tp, XFS_TRANS_SB_FDBLOCKS, id.nfree);
> +
> +	/*
> +	 * update in-core counters (especially sb_fdblocks) now
> +	 * so xfs_validate_sb_write() can pass.
> +	 */
> +	if (xfs_sb_version_haslazysbcount(&mp->m_sb))
> +		xfs_log_sb(tp);
> +
>  	xfs_trans_set_sync(tp);
>  	error = xfs_trans_commit(tp);
>  	if (error)
> @@ -178,6 +209,10 @@ xfs_growfs_data_private(
>  	return error;
>  
>  out_trans_cancel:
> +	if (!extend && (tp->t_flags & XFS_TRANS_DIRTY)) {
> +		xfs_trans_commit(tp);
> +		return error;

When do we encounter the (!extend && DIRTY && cancelled) state?

--D

> +	}
>  	xfs_trans_cancel(tp);
>  	return error;
>  }
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index e72730f85af1..fd2cbf414b80 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -419,7 +419,6 @@ xfs_trans_mod_sb(
>  		tp->t_res_frextents_delta += delta;
>  		break;
>  	case XFS_TRANS_SB_DBLOCKS:
> -		ASSERT(delta > 0);
>  		tp->t_dblocks_delta += delta;
>  		break;
>  	case XFS_TRANS_SB_AGCOUNT:
> -- 
> 2.27.0
> 

Re: [PATCH v3 4/4] xfs: support shrinking unused space in the last AG

[Gao Xiang]

Hi Darrick,

On Fri, Jan 08, 2021 at 01:19:42PM -0800, Darrick J. Wong wrote:
> On Sat, Jan 09, 2021 at 03:09:19AM +0800, Gao Xiang wrote:

...

> >  	delta = nb;	/* use delta as a temporary here */
> 
> Yikes, can this become a separate variable please?

Ok.

> 
> >  	nb_mod = do_div(delta, mp->m_sb.sb_agblocks);
> > @@ -99,10 +102,18 @@ xfs_growfs_data_private(
> >  	if (nb_mod && nb_mod < XFS_MIN_AG_BLOCKS) {
> >  		nagcount--;
> >  		nb = (xfs_rfsblock_t)nagcount * mp->m_sb.sb_agblocks;
> > -		if (nb < mp->m_sb.sb_dblocks)
> > +		if (nagcount < 2)
> >  			return -EINVAL;
> >  	}
> > -	delta = nb - mp->m_sb.sb_dblocks;
> > +
> > +	if (nb > mp->m_sb.sb_dblocks) {
> > +		delta = nb - mp->m_sb.sb_dblocks;
> > +		extend = true;
> > +	} else {
> > +		delta = mp->m_sb.sb_dblocks - nb;
> > +		extend = false;
> 
> /me wonders why delta isn't simply int64_t, and then you can do things
> like:
> 
> if (delta > 0)
> 	growfs
> else if (delta < 0)
> 	shrinkfs
> 
> ?

Yeah, delta changed into a signed variable in mycurrent experimental
shrinking entire AG hack as well. Will update this here in advance.

Thanks,
Gao Xiang