diff mbox series

[v8,2/5] xfs: hoist out xfs_resizefs_init_new_ags()

Message ID 20210305025703.3069469-3-hsiangkao@redhat.com (mailing list archive)
State Superseded
Headers show
Series xfs: support shrinking free space in the last AG | expand

Commit Message

Gao Xiang March 5, 2021, 2:57 a.m. UTC
Move out related logic for initializing new added AGs to a new helper
in preparation for shrinking. No logic changes.

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
 fs/xfs/xfs_fsops.c | 107 +++++++++++++++++++++++++++------------------
 1 file changed, 64 insertions(+), 43 deletions(-)

Comments

Brian Foster March 22, 2021, 11:28 a.m. UTC | #1
On Fri, Mar 05, 2021 at 10:57:00AM +0800, Gao Xiang wrote:
> Move out related logic for initializing new added AGs to a new helper
> in preparation for shrinking. No logic changes.
> 
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
>  fs/xfs/xfs_fsops.c | 107 +++++++++++++++++++++++++++------------------
>  1 file changed, 64 insertions(+), 43 deletions(-)
> 
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index 9f9ba8bd0213..fc9e799b2ae3 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/fs/xfs/xfs_fsops.c
> @@ -20,6 +20,64 @@
>  #include "xfs_ag.h"
>  #include "xfs_ag_resv.h"
>  
> +/*
> + * 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.
> + */
> +static int
> +xfs_resizefs_init_new_ags(
> +	struct xfs_trans	*tp,
> +	struct aghdr_init_data	*id,
> +	xfs_agnumber_t		oagcount,
> +	xfs_agnumber_t		nagcount,
> +	xfs_rfsblock_t		delta,
> +	bool			*lastag_resetagres)

Nit: I'd just call this lastag_extended or something that otherwise
indicates what this function reports (as opposed to trying to tell the
caller what to do).

> +{
> +	struct xfs_mount	*mp = tp->t_mountp;
> +	xfs_rfsblock_t		nb = mp->m_sb.sb_dblocks + delta;
> +	int			error;
> +
> +	*lastag_resetagres = false;
> +
> +	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;
> +		}
> +	}
> +
> +	error = xfs_buf_delwri_submit(&id->buffer_list);
> +	if (error)
> +		return error;
> +
> +	xfs_trans_agblocks_delta(tp, id->nfree);
> +
> +	if (delta) {
> +		*lastag_resetagres = true;
> +		error = xfs_ag_extend_space(mp, tp, id, delta);
> +	}
> +	return error;
> +}
> +
>  /*
>   * growfs operations
>   */
...
> @@ -123,9 +145,8 @@ 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)
> -		xfs_trans_mod_sb(tp, XFS_TRANS_SB_DBLOCKS,
> -				 nb - mp->m_sb.sb_dblocks);
> +	if (delta > 0)
> +		xfs_trans_mod_sb(tp, XFS_TRANS_SB_DBLOCKS, delta);

Hm.. isn't delta still unsigned as of this patch?

Brian

>  	if (id.nfree)
>  		xfs_trans_mod_sb(tp, XFS_TRANS_SB_FDBLOCKS, id.nfree);
>  
> @@ -152,7 +173,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 (delta) {
> +	if (lastag_resetagres) {
>  		struct xfs_perag	*pag;
>  
>  		pag = xfs_perag_get(mp, id.agno);
> -- 
> 2.27.0
>
Gao Xiang March 22, 2021, 11:53 a.m. UTC | #2
On Mon, Mar 22, 2021 at 07:28:07AM -0400, Brian Foster wrote:
> On Fri, Mar 05, 2021 at 10:57:00AM +0800, Gao Xiang wrote:
> > Move out related logic for initializing new added AGs to a new helper
> > in preparation for shrinking. No logic changes.
> > 
> > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > ---
> >  fs/xfs/xfs_fsops.c | 107 +++++++++++++++++++++++++++------------------
> >  1 file changed, 64 insertions(+), 43 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> > index 9f9ba8bd0213..fc9e799b2ae3 100644
> > --- a/fs/xfs/xfs_fsops.c
> > +++ b/fs/xfs/xfs_fsops.c
> > @@ -20,6 +20,64 @@
> >  #include "xfs_ag.h"
> >  #include "xfs_ag_resv.h"
> >  
> > +/*
> > + * 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.
> > + */
> > +static int
> > +xfs_resizefs_init_new_ags(
> > +	struct xfs_trans	*tp,
> > +	struct aghdr_init_data	*id,
> > +	xfs_agnumber_t		oagcount,
> > +	xfs_agnumber_t		nagcount,
> > +	xfs_rfsblock_t		delta,
> > +	bool			*lastag_resetagres)
> 
> Nit: I'd just call this lastag_extended or something that otherwise
> indicates what this function reports (as opposed to trying to tell the
> caller what to do).

ok, if other people don't oppose of it.

> 
> > +{
> > +	struct xfs_mount	*mp = tp->t_mountp;
> > +	xfs_rfsblock_t		nb = mp->m_sb.sb_dblocks + delta;
> > +	int			error;
> > +
> > +	*lastag_resetagres = false;
> > +
> > +	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;
> > +		}
> > +	}
> > +
> > +	error = xfs_buf_delwri_submit(&id->buffer_list);
> > +	if (error)
> > +		return error;
> > +
> > +	xfs_trans_agblocks_delta(tp, id->nfree);
> > +
> > +	if (delta) {
> > +		*lastag_resetagres = true;
> > +		error = xfs_ag_extend_space(mp, tp, id, delta);
> > +	}
> > +	return error;
> > +}
> > +
> >  /*
> >   * growfs operations
> >   */
> ...
> > @@ -123,9 +145,8 @@ 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)
> > -		xfs_trans_mod_sb(tp, XFS_TRANS_SB_DBLOCKS,
> > -				 nb - mp->m_sb.sb_dblocks);
> > +	if (delta > 0)
> > +		xfs_trans_mod_sb(tp, XFS_TRANS_SB_DBLOCKS, delta);
> 
> Hm.. isn't delta still unsigned as of this patch?

Not sure if some difference exists, I could update it as "if (delta)",
therefore it seems [PATCH v8 4/5] won't modify this anymore.

Thanks,
Gao Xiang

> 
> Brian
> 
> >  	if (id.nfree)
> >  		xfs_trans_mod_sb(tp, XFS_TRANS_SB_FDBLOCKS, id.nfree);
> >  
> > @@ -152,7 +173,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 (delta) {
> > +	if (lastag_resetagres) {
> >  		struct xfs_perag	*pag;
> >  
> >  		pag = xfs_perag_get(mp, id.agno);
> > -- 
> > 2.27.0
> > 
>
Brian Foster March 22, 2021, 12:21 p.m. UTC | #3
On Mon, Mar 22, 2021 at 07:53:49PM +0800, Gao Xiang wrote:
> On Mon, Mar 22, 2021 at 07:28:07AM -0400, Brian Foster wrote:
> > On Fri, Mar 05, 2021 at 10:57:00AM +0800, Gao Xiang wrote:
> > > Move out related logic for initializing new added AGs to a new helper
> > > in preparation for shrinking. No logic changes.
> > > 
> > > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > > ---
> > >  fs/xfs/xfs_fsops.c | 107 +++++++++++++++++++++++++++------------------
> > >  1 file changed, 64 insertions(+), 43 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> > > index 9f9ba8bd0213..fc9e799b2ae3 100644
> > > --- a/fs/xfs/xfs_fsops.c
> > > +++ b/fs/xfs/xfs_fsops.c
...
> > > @@ -123,9 +145,8 @@ 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)
> > > -		xfs_trans_mod_sb(tp, XFS_TRANS_SB_DBLOCKS,
> > > -				 nb - mp->m_sb.sb_dblocks);
> > > +	if (delta > 0)
> > > +		xfs_trans_mod_sb(tp, XFS_TRANS_SB_DBLOCKS, delta);
> > 
> > Hm.. isn't delta still unsigned as of this patch?
> 
> Not sure if some difference exists, I could update it as "if (delta)",
> therefore it seems [PATCH v8 4/5] won't modify this anymore.
> 

Yeah, not sure it's a problem, it just stands out a bit with it being in
a separate patch from the change to the variable type.

Brian

> Thanks,
> Gao Xiang
> 
> > 
> > Brian
> > 
> > >  	if (id.nfree)
> > >  		xfs_trans_mod_sb(tp, XFS_TRANS_SB_FDBLOCKS, id.nfree);
> > >  
> > > @@ -152,7 +173,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 (delta) {
> > > +	if (lastag_resetagres) {
> > >  		struct xfs_perag	*pag;
> > >  
> > >  		pag = xfs_perag_get(mp, id.agno);
> > > -- 
> > > 2.27.0
> > > 
> > 
>
diff mbox series

Patch

diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index 9f9ba8bd0213..fc9e799b2ae3 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -20,6 +20,64 @@ 
 #include "xfs_ag.h"
 #include "xfs_ag_resv.h"
 
+/*
+ * 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.
+ */
+static int
+xfs_resizefs_init_new_ags(
+	struct xfs_trans	*tp,
+	struct aghdr_init_data	*id,
+	xfs_agnumber_t		oagcount,
+	xfs_agnumber_t		nagcount,
+	xfs_rfsblock_t		delta,
+	bool			*lastag_resetagres)
+{
+	struct xfs_mount	*mp = tp->t_mountp;
+	xfs_rfsblock_t		nb = mp->m_sb.sb_dblocks + delta;
+	int			error;
+
+	*lastag_resetagres = false;
+
+	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;
+		}
+	}
+
+	error = xfs_buf_delwri_submit(&id->buffer_list);
+	if (error)
+		return error;
+
+	xfs_trans_agblocks_delta(tp, id->nfree);
+
+	if (delta) {
+		*lastag_resetagres = true;
+		error = xfs_ag_extend_space(mp, tp, id, delta);
+	}
+	return error;
+}
+
 /*
  * growfs operations
  */
@@ -34,6 +92,7 @@  xfs_growfs_data_private(
 	xfs_agnumber_t		nagimax = 0;
 	xfs_rfsblock_t		nb, nb_div, nb_mod;
 	xfs_rfsblock_t		delta;
+	bool			lastag_resetagres;
 	xfs_agnumber_t		oagcount;
 	struct xfs_trans	*tp;
 	struct aghdr_init_data	id = {};
@@ -74,48 +133,11 @@  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(tp, &id, oagcount, nagcount,
+					  delta, &lastag_resetagres);
 	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 (delta) {
-		error = xfs_ag_extend_space(mp, tp, &id, delta);
-		if (error)
-			goto out_trans_cancel;
-	}
-
 	/*
 	 * Update changed superblock fields transactionally. These are not
 	 * seen by the rest of the world until the transaction commit applies
@@ -123,9 +145,8 @@  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)
-		xfs_trans_mod_sb(tp, XFS_TRANS_SB_DBLOCKS,
-				 nb - mp->m_sb.sb_dblocks);
+	if (delta > 0)
+		xfs_trans_mod_sb(tp, XFS_TRANS_SB_DBLOCKS, delta);
 	if (id.nfree)
 		xfs_trans_mod_sb(tp, XFS_TRANS_SB_FDBLOCKS, id.nfree);
 
@@ -152,7 +173,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 (delta) {
+	if (lastag_resetagres) {
 		struct xfs_perag	*pag;
 
 		pag = xfs_perag_get(mp, id.agno);