headers
Christoph Hellwig | 4 Jul 2012 16:54
Favicon

[PATCH 0/7] inode allocator refactoring V2

This series turns the higher level inode allocator upside down.

The biggest change is that we try to operate on the incore perag
structure as much as possible instead of reading the AGI buffer.

I don't have a system to measure the benefit on the large create benchmarks
right now, but even if it's not benefitial it at least greatly cleans up
the code.

Changes since V1:
  - minor cleanups noted by Dave

Note that this does not collapse the three passes in xfs_dialloc yet -
I tried it and got deadlocks that I haven't fully understood yet.  I plan
to look into them when I get a bit more time.

_______________________________________________
xfs mailing list
xfs <at> oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
Permalink | Reply |
headers
Christoph Hellwig | 4 Jul 2012 16:54
Favicon

[PATCH 3/7] xfs: remove the alloc_done argument to xfs_dialloc

We can simplify check the IO_agbp pointer for being non-NULL instead of
passing another argument through two layers of function calls.

Reviewed-by: Dave Chinner <dchinner <at> redhat.com>
Signed-off-by: Christoph Hellwig <hch <at> lst.de>

---
 fs/xfs/xfs_ialloc.c |    3 ---
 fs/xfs/xfs_ialloc.h |    2 --
 fs/xfs/xfs_inode.c  |    5 ++---
 fs/xfs/xfs_inode.h  |    2 +-
 fs/xfs/xfs_utils.c  |   17 +++++++----------
 5 files changed, 10 insertions(+), 19 deletions(-)

Index: xfs/fs/xfs/xfs_ialloc.c
===================================================================
--- xfs.orig/fs/xfs/xfs_ialloc.c	2012-07-02 12:23:27.000000000 +0200
+++ xfs/fs/xfs/xfs_ialloc.c	2012-07-02 12:23:49.299108893 +0200
 <at>  <at>  -895,7 +895,6  <at>  <at>  xfs_dialloc(
 	umode_t			mode,
 	int			okalloc,
 	struct xfs_buf		**IO_agbp,
-	boolean_t		*alloc_done,
 	xfs_ino_t		*inop)
 {
 	struct xfs_buf		*agbp;
 <at>  <at>  -955,7 +954,6  <at>  <at>  xfs_dialloc(
 	 * or in which we can allocate some inodes.  Iterate through the
 	 * allocation groups upward, wrapping at the end.
 	 */
(Continue reading)

Permalink | Reply |
headers
Mark Tinguely | 26 Jul 2012 19:48
Picon
Favicon

Re: [PATCH 3/7] xfs: remove the alloc_done argument to xfs_dialloc

On 07/04/12 09:54, Christoph Hellwig wrote:
> We can simplify check the IO_agbp pointer for being non-NULL instead of
> passing another argument through two layers of function calls.
>
> Reviewed-by: Dave Chinner<dchinner <at> redhat.com>
> Signed-off-by: Christoph Hellwig<hch <at> lst.de>

looks good.

Reviewed-by: Mark Tinguely <tinguely <at> sgi.com>

_______________________________________________
xfs mailing list
xfs <at> oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
Permalink | Reply |
headers
Christoph Hellwig | 4 Jul 2012 16:54
Favicon

[PATCH 4/7] xfs: add a short cut to xfs_dialloc for the non-NULL agbp case

In this case we already have selected an AG and know it has free space
beause the buffer lock never got released.  Jump directly into xfs_dialloc_ag
and short cut the AG selection loop.

Reviewed-by: Dave Chinner <dchinner <at> redhat.com>
Signed-off-by: Christoph Hellwig <hch <at> lst.de>

---
 fs/xfs/xfs_ialloc.c |   47 ++++++++++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 21 deletions(-)

Index: xfs/fs/xfs/xfs_ialloc.c
===================================================================
--- xfs.orig/fs/xfs/xfs_ialloc.c	2012-07-02 12:23:49.299108893 +0200
+++ xfs/fs/xfs/xfs_ialloc.c	2012-07-02 12:24:18.305775386 +0200
 <at>  <at>  -634,6 +634,10  <at>  <at>  xfs_dialloc_ag(

 	pag = xfs_perag_get(mp, agno);

+	ASSERT(pag->pagi_init);
+	ASSERT(pag->pagi_inodeok);
+	ASSERT(pag->pagi_freecount > 0);
+
  restart_pagno:
 	cur = xfs_inobt_init_cursor(mp, tp, agbp, agno);
 	/*
 <at>  <at>  -907,32 +911,32  <at>  <at>  xfs_dialloc(
 	xfs_agnumber_t		tagno;
 	struct xfs_perag	*pag;
(Continue reading)

Permalink | Reply |
headers
Mark Tinguely | 26 Jul 2012 19:50
Picon
Favicon

Re: [PATCH 4/7] xfs: add a short cut to xfs_dialloc for the non-NULL agbp case

On 07/04/12 09:54, Christoph Hellwig wrote:
> In this case we already have selected an AG and know it has free space
> beause the buffer lock never got released.  Jump directly into xfs_dialloc_ag
> and short cut the AG selection loop.
>
> Reviewed-by: Dave Chinner<dchinner <at> redhat.com>
> Signed-off-by: Christoph Hellwig<hch <at> lst.de>
>
> ---

Looks good.

Reviewed-by: Mark Tinguely <tinguely <at> sgi.com>

_______________________________________________
xfs mailing list
xfs <at> oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
Permalink | Reply |
headers
Christoph Hellwig | 4 Jul 2012 16:54
Favicon

[PATCH 7/7] xfs: merge xfs_ialloc_ag_select into xfs_dialloc

Both function contain the same basic loop over all AGs.  Merge the two
by creating three passes in the loop instead of duplicating the code.

Signed-off-by: Christoph Hellwig <hch <at> lst.de>

---
 fs/xfs/xfs_ialloc.c |  179 +++++++++++++++-------------------------------------
 1 file changed, 55 insertions(+), 124 deletions(-)

Index: xfs/fs/xfs/xfs_ialloc.c
===================================================================
--- xfs.orig/fs/xfs/xfs_ialloc.c	2012-07-02 12:25:24.365774992 +0200
+++ xfs/fs/xfs/xfs_ialloc.c	2012-07-02 12:26:14.325774734 +0200
 <at>  <at>  -439,114 +439,6  <at>  <at>  xfs_ialloc_next_ag(
 }

 /*
- * Select an allocation group to look for a free inode in, based on the parent
- * inode and then mode.  Return the allocation group buffer.
- */
-STATIC xfs_agnumber_t
-xfs_ialloc_ag_select(
-	xfs_trans_t	*tp,		/* transaction pointer */
-	xfs_ino_t	parent,		/* parent directory inode number */
-	umode_t		mode,		/* bits set to indicate file type */
-	int		okalloc)	/* ok to allocate more space */
-{
-	xfs_agnumber_t	agcount;	/* number of ag's in the filesystem */
-	xfs_agnumber_t	agno;		/* current ag number */
-	int		flags;		/* alloc buffer locking flags */
(Continue reading)

Permalink | Reply |
headers
Mark Tinguely | 26 Jul 2012 19:47
Picon
Favicon

Re: [PATCH 7/7] xfs: merge xfs_ialloc_ag_select into xfs_dialloc


+		if (pass < 2) {
+			/*
+			 * Is there enough free space for the file plus a block
+			 * of inodes?
+			 */
+			xfs_extlen_t	longest = pag->pagf_longest;
+			int		needspace =
+				S_ISDIR(mode) || S_ISREG(mode) || S_ISLNK(mode);
+
+			if (!longest)
+				longest = pag->pagf_flcount > 0;
+
+			if (pag->pagf_freeblks <
+			    XFS_IALLOC_BLOCKS(mp) + needspace)
+				goto nextag;
				^^^^^^^ here

+			if (longest < XFS_IALLOC_BLOCKS(mp))
+				goto nextag;

				^^^^^^^ and here
+		}

Isn't the agbp locked from the earlier xfs_ialloc_read_agi()?
Do we want to release them before going on to the next AG?

Thank-you,

--Mark.
(Continue reading)

Permalink | Reply |
headers
Mark Tinguely | 30 Jul 2012 19:21
Picon
Favicon

Re: [PATCH 7/7] xfs: merge xfs_ialloc_ag_select into xfs_dialloc

On 07/26/12 12:47, Mark Tinguely wrote:
>
>
> + if (pass < 2) {
> + /*
> + * Is there enough free space for the file plus a block
> + * of inodes?
> + */
> + xfs_extlen_t longest = pag->pagf_longest;
> + int needspace =
> + S_ISDIR(mode) || S_ISREG(mode) || S_ISLNK(mode);
> +
> + if (!longest)
> + longest = pag->pagf_flcount > 0;
> +
> + if (pag->pagf_freeblks <
> + XFS_IALLOC_BLOCKS(mp) + needspace)
> + goto nextag;
> ^^^^^^^ here
>
> + if (longest < XFS_IALLOC_BLOCKS(mp))
> + goto nextag;
>
> ^^^^^^^ and here
> + }
>
> Isn't the agbp locked from the earlier xfs_ialloc_read_agi()?
> Do we want to release them before going on to the next AG?
>
> Thank-you,
(Continue reading)

Permalink | Reply |
headers
Christoph Hellwig | 4 Jul 2012 16:54
Favicon

[PATCH 2/7] xfs: split xfs_dialloc

Move the actual allocation once we have selected an allocation group into a
separate helper, and make xfs_dialloc a wrapper around it.

Reviewed-by: Dave Chinner <dchinner <at> redhat.com>
Signed-off-by: Christoph Hellwig <hch <at> lst.de>

---
 fs/xfs/xfs_ialloc.c |  349 +++++++++++++++++++++++++---------------------------
 1 file changed, 174 insertions(+), 175 deletions(-)

Index: xfs/fs/xfs/xfs_ialloc.c
===================================================================
--- xfs.orig/fs/xfs/xfs_ialloc.c	2012-07-02 12:14:21.832445616 +0200
+++ xfs/fs/xfs/xfs_ialloc.c	2012-07-02 12:23:27.395775691 +0200
 <at>  <at>  -607,188 +607,35  <at>  <at>  xfs_ialloc_get_rec(
 }

 /*
- * Visible inode allocation functions.
- */
-
-/*
- * Allocate an inode on disk.
- * Mode is used to tell whether the new inode will need space, and whether
- * it is a directory.
+ * Allocate an inode.
  *
- * The arguments IO_agbp and alloc_done are defined to work within
- * the constraint of one allocation per transaction.
- * xfs_dialloc() is designed to be called twice if it has to do an
(Continue reading)

Permalink | Reply |
headers
Ben Myers | 29 Jul 2012 22:55
Picon
Favicon

Re: [PATCH 2/7] xfs: split xfs_dialloc

On Wed, Jul 04, 2012 at 10:54:46AM -0400, Christoph Hellwig wrote:
> Move the actual allocation once we have selected an allocation group into a
> separate helper, and make xfs_dialloc a wrapper around it.
> 
> Reviewed-by: Dave Chinner <dchinner <at> redhat.com>
> Signed-off-by: Christoph Hellwig <hch <at> lst.de>

Looks fine.

Reviewed-by: Ben Myers <bpm <at> sgi.com>

_______________________________________________
xfs mailing list
xfs <at> oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
Permalink | Reply |
headers
Christoph Hellwig | 4 Jul 2012 16:54
Favicon

[PATCH 5/7] xfs: refactor xfs_ialloc_ag_select

Loop over the in-core perag structures and prefer using pagi_freecount over
going out to the AGI buffer where possible.

Reviewed-by: Dave Chinner <dchinner <at> redhat.com>
Signed-off-by: Christoph Hellwig <hch <at> lst.de>

---
 fs/xfs/xfs_ialloc.c |   95 ++++++++++++++++++++++++----------------------------
 1 file changed, 44 insertions(+), 51 deletions(-)

Index: xfs/fs/xfs/xfs_ialloc.c
===================================================================
--- xfs.orig/fs/xfs/xfs_ialloc.c	2012-07-02 12:24:18.305775386 +0200
+++ xfs/fs/xfs/xfs_ialloc.c	2012-07-02 12:24:50.605775194 +0200
 <at>  <at>  -442,14 +442,13  <at>  <at>  xfs_ialloc_next_ag(
  * Select an allocation group to look for a free inode in, based on the parent
  * inode and then mode.  Return the allocation group buffer.
  */
-STATIC xfs_buf_t *			/* allocation group buffer */
+STATIC xfs_agnumber_t
 xfs_ialloc_ag_select(
 	xfs_trans_t	*tp,		/* transaction pointer */
 	xfs_ino_t	parent,		/* parent directory inode number */
 	umode_t		mode,		/* bits set to indicate file type */
 	int		okalloc)	/* ok to allocate more space */
 {
-	xfs_buf_t	*agbp;		/* allocation group header buffer */
 	xfs_agnumber_t	agcount;	/* number of ag's in the filesystem */
 	xfs_agnumber_t	agno;		/* current ag number */
 	int		flags;		/* alloc buffer locking flags */
(Continue reading)

Permalink | Reply |
headers
Mark Tinguely | 26 Jul 2012 19:53
Picon
Favicon

Re: [PATCH 5/7] xfs: refactor xfs_ialloc_ag_select

On 07/04/12 09:54, Christoph Hellwig wrote:
> Loop over the in-core perag structures and prefer using pagi_freecount over
> going out to the AGI buffer where possible.
>
> Reviewed-by: Dave Chinner<dchinner <at> redhat.com>
> Signed-off-by: Christoph Hellwig<hch <at> lst.de>

Looks good

Not your fault, but the "longest" variable is complicated to understand.

Reviewed-by: Mark Tinguely <tinguely <at> sgi.com>

_______________________________________________
xfs mailing list
xfs <at> oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
Permalink | Reply |
headers
Christoph Hellwig | 4 Jul 2012 16:54
Favicon

[PATCH 1/7] xfs: remove xfs_ialloc_find_free

This function is entirely trivial and only has one caller, so remove it to
simplify the code.

Reviewed-by: Dave Chinner <dchinner <at> redhat.com>
Signed-off-by: Christoph Hellwig <hch <at> lst.de>

---
 fs/xfs/xfs_ialloc.c |    9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

Index: xfs/fs/xfs/xfs_ialloc.c
===================================================================
--- xfs.orig/fs/xfs/xfs_ialloc.c	2012-06-04 12:39:42.512235078 -0400
+++ xfs/fs/xfs/xfs_ialloc.c	2012-06-04 12:39:44.012235117 -0400
 <at>  <at>  -609,13 +609,6  <at>  <at>  xfs_ialloc_get_rec(
 /*
  * Visible inode allocation functions.
  */
-/*
- * Find a free (set) bit in the inode bitmask.
- */
-static inline int xfs_ialloc_find_free(xfs_inofree_t *fp)
-{
-	return xfs_lowbit64(*fp);
-}

 /*
  * Allocate an inode on disk.
 <at>  <at>  -995,7 +988,7  <at>  <at>  newino:
 	}
(Continue reading)

Permalink | Reply |
headers
Christoph Hellwig | 4 Jul 2012 16:54
Favicon

[PATCH 6/7] xfs: do not read the AGI buffer in xfs_dialloc until nessecary

Refactor the AG selection loop in xfs_dialloc to operate on the in-memory
perag data as much as possible.  We only read the AGI buffer once we have
selected an AG to allocate inodes now instead of for every AG considered.

Reviewed-by: Dave Chinner <dchinner <at> redhat.com>
Signed-off-by: Christoph Hellwig <hch <at> lst.de>

---
 fs/xfs/xfs_ialloc.c |  127 ++++++++++++++++++++++++++++------------------------
 1 file changed, 69 insertions(+), 58 deletions(-)

Index: xfs/fs/xfs/xfs_ialloc.c
===================================================================
--- xfs.orig/fs/xfs/xfs_ialloc.c	2012-07-02 12:24:50.000000000 +0200
+++ xfs/fs/xfs/xfs_ialloc.c	2012-07-02 12:25:24.365774992 +0200
 <at>  <at>  -900,11 +900,10  <at>  <at>  xfs_dialloc(
 	struct xfs_mount	*mp = tp->t_mountp;
 	struct xfs_buf		*agbp;
 	xfs_agnumber_t		agno;
-	struct xfs_agi		*agi;
 	int			error;
 	int			ialloced;
 	int			noroom = 0;
-	xfs_agnumber_t		tagno;
+	xfs_agnumber_t		start_agno;
 	struct xfs_perag	*pag;

 	if (*IO_agbp) {
 <at>  <at>  -921,25 +920,17  <at>  <at>  xfs_dialloc(
 	 * We do not have an agbp, so select an initial allocation
(Continue reading)

Permalink | Reply |
headers
Mark Tinguely | 26 Jul 2012 19:53
Picon
Favicon

Re: [PATCH 6/7] xfs: do not read the AGI buffer in xfs_dialloc until nessecary

On 07/04/12 09:54, Christoph Hellwig wrote:
> Refactor the AG selection loop in xfs_dialloc to operate on the in-memory
> perag data as much as possible.  We only read the AGI buffer once we have
> selected an AG to allocate inodes now instead of for every AG considered.
>
> Reviewed-by: Dave Chinner<dchinner <at> redhat.com>
> Signed-off-by: Christoph Hellwig<hch <at> lst.de>

Looks good.

Reviewed-by: Mark Tinguely <tinguely <at> sgi.com>

_______________________________________________
xfs mailing list
xfs <at> oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
Permalink | Reply |

Gmane