[PATCH 2/2] xfs_spaceman: Accounting for AGFL blocks

[PATCH 2/2] xfs_spaceman: Accounting for AGFL blocks

[Dhruvesh Rathore]

Here is the original discussion that Dave wrote for xfs_spaceman:

http://oss.sgi.com/archives/xfs/2012-10/msg00363.html

These patches were not posted and were just forward ported to a current 
3.18-rc7+ for-next XFS tree and were pushed to the fiemapfs branch in Dave's
kernel tree at:

git://git.kernel.org/pub/scm/linux/kernel/git/dgc/linux-xfs.git

The original xfs_spaceman tool that Dave wrote is here:

http://oss.sgi.com/archives/xfs/2012-10/msg00366.html

Dave just updated it to the 3.2.2 code base and pushed it to the
spaceman branch in this tree:

git://git.kernel.org/pub/scm/fs/xfs/xfsprogs-dev.git

This xfs_spaceman utility previously failed to account for AGFL blocks.
Old output (Before changes).

$ sudo xfs_spaceman -c "freesp" /media/xfs
   from      to extents  blocks    pct
   1024    2047       1    1262   0.04
   4096    8191       1    5126   0.15
   8192   16383       3   35344   1.05
  32768   65535       1   43989   1.31
 262144  524287       3 1334894  39.78
 524288  967428       2 1934840  57.66

As you can see the AGFL blocks were unaccounted (4 per AG, and there 
were 4 AGs in this filesystem).

This patch is concerned with adding a new function which will walk the free
extents in AGFL and account for these AGFL blocks, while file system scanning.

New output (After implementing this patch).

$ uname -a
Linux dhruv-MacBookAir 3.18.0-rc7+ #3 SMP Thu Dec 25 15:29:59 IST 2014 x86_64 x86_64 x86_64 GNU/Linux
$ sudo xfs_spaceman -V
xfs_spaceman version 3.2.2
$ sudo xfs_spaceman -c "freesp" /media/xfs
   from      to extents  blocks    pct
      1       1      16      16   0.00
   1024    2047       1    1262   0.04
   4096    8191       1    5126   0.15
   8192   16383       3   35344   1.05
  32768   65535       1   43989   1.31
 262144  524287       3 1334894  39.78
 524288  967428       2 1934840  57.66

Please do comment.

Signed-off-by: 
Dhruvesh Rathore <dhruvesh_r@outlook.com>
Amey Ruikar <ameyruikar@yahoo.com>
Somdeep Dey <somdeepdey10@gmail.com>
---
linux-xfs/fs/xfs/libxfs/xfs_alloc.c | 81 +++
1 file changed, 81 insertions(+)

-------------------------------------------------------------------------------------------

--- a/linux-xfs/fs/xfs/libxfs/xfs_alloc.c	2015-01-08 19:34:52.927862215 +0530
+++ b/linux-xfs/fs/xfs/libxfs/xfs_alloc.c	2015-01-09 14:25:23.853599350 +0530
@@ -2697,6 +2697,83 @@
 
 }
 
+
+/*
+ * Walk the free extents in the AGFL (AG Free List) of each AG, and dump
+ * them all into the fieinfo.
+ * 
+ * With a freshly made filesystem, 4 blocks are reserved immediately after
+ * the free space B+tree root blocks (blocks 4 to 7). As they are used up
+ * additional blocks are reserved from the AG and added to the free list
+ * array. A typical device will have space for 128 elements in the array.
+ * The actual size can be determined using XFS_AGFL_SIZE macro. The array
+ * is maintained as a circular list and active elements are pointed by 
+ * AGF's agf_flfirst, agf_fllast and agf_flcount values.
+ */
+static int
+xfs_alloc_agfl_freespace_map(
+	struct xfs_buf		**agbp,	/* buffer for a.g. freelist header */
+	struct xfs_btree_cur    *cur,
+	struct fiemap_extent_info *fieinfo,
+	xfs_agblock_t		sagbno,
+	xfs_agblock_t		eagbno)
+{
+	xfs_agf_t	*agf;		/* a.g. freespace structure */
+	xfs_buf_t	*agflbp;	/* buffer for a.g. freelist structure */
+	__be32		*agfl_bno;	/* Reference to freelist block */
+	int	 	j;
+	int 		index;
+	int 		error = 0;
+
+	agf = XFS_BUF_TO_AGF(*agbp);
+	error = xfs_alloc_read_agfl(cur->bc_mp, NULL, be32_to_cpu(agf->agf_seqno),
+				    &agflbp);
+	if (error)
+		return error;
+
+	agfl_bno = XFS_BUF_TO_AGFL_BNO(cur->bc_mp, agflbp);
+	
+	index = be32_to_cpu(agf->agf_flfirst);
+	
+	for(j=1 ; j<=be32_to_cpu(agf->agf_flcount); j++)
+        {
+	 xfs_agblock_t	fbno;
+	 xfs_extlen_t	flen;
+	 xfs_daddr_t	dbno;
+	 xfs_fileoff_t	dlen;	
+	 int		flags = 0;
+
+	/* Relative AG block number */
+	 fbno = be32_to_cpu(agfl_bno[index]);
+	/* Each entry in the AGFL is a single entry i.e length is 1 block */ 
+	 flen = 1;
+
+	/*
+	 * AGFL is maintained as a circular list. Following is needed
+	 * to handle array wrapping correctly.
+	 */	
+         if( index == ( (XFS_AGFL_SIZE(cur->bc_mp))-1 ) )
+	  index = 0;	/* First index of AGFL */
+         else
+          index++;	/* Next index in AGFL */
+        
+	/*
+	 * Use daddr format for all range/len calculations as that is
+	 * the format the range/len variables are supplied in by
+	 * userspace.
+	 */
+	 dbno = XFS_AGB_TO_DADDR(cur->bc_mp, cur->bc_private.a.agno, fbno);
+	 dlen = XFS_FSB_TO_BB(cur->bc_mp, flen);
+	 error = -fiemap_fill_next_extent(fieinfo, BBTOB(dbno),
+			                  BBTOB(dbno), BBTOB(dlen), flags);
+	 if (error)
+	   break;
+	}
+	xfs_buf_relse(agflbp);	/* Release the buffer */
+	return error;
+
+}
+
 /*
  * Map the freespace from the requested range in the requested order.
  *
@@ -2802,6 +2879,10 @@
 		}
 		XFS_WANT_CORRUPTED_GOTO(i == 1, del_cursor);
 
+		/* Account for the free blocks in AGFL */		
+		error = xfs_alloc_agfl_freespace_map(&agbp, cur, fieinfo, sagno,
+					agno == eagno ? eagbno : NULLAGBLOCK);
+
 		if (!bycnt) {
 			/*
 			 * if we are doing a bno ordered lookup, we can just

------------------------------------------------------------------------------------------------

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs_spaceman: Accounting for AGFL blocks

[Dave Chinner]

On Fri, Jan 23, 2015 at 09:04:00AM +0530, Dhruvesh Rathore wrote:
> Here is the original discussion that Dave wrote for xfs_spaceman:
> 
> http://oss.sgi.com/archives/xfs/2012-10/msg00363.html
> 
> These patches were not posted and were just forward ported to a current 
> 3.18-rc7+ for-next XFS tree and were pushed to the fiemapfs branch in Dave's
> kernel tree at:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/dgc/linux-xfs.git
> 
> The original xfs_spaceman tool that Dave wrote is here:
> 
> http://oss.sgi.com/archives/xfs/2012-10/msg00366.html
> 
> Dave just updated it to the 3.2.2 code base and pushed it to the
> spaceman branch in this tree:
> 
> git://git.kernel.org/pub/scm/fs/xfs/xfsprogs-dev.git

As per last patch, this is series description so it doesn't need to
be repeated in every patch ;)

>From here:

> This xfs_spaceman utility previously failed to account for AGFL blocks.
> Old output (Before changes).
> 
> $ sudo xfs_spaceman -c "freesp" /media/xfs
>    from      to extents  blocks    pct
>    1024    2047       1    1262   0.04
>    4096    8191       1    5126   0.15
>    8192   16383       3   35344   1.05
>   32768   65535       1   43989   1.31
>  262144  524287       3 1334894  39.78
>  524288  967428       2 1934840  57.66
> 
> As you can see the AGFL blocks were unaccounted (4 per AG, and there 
> were 4 AGs in this filesystem).
> 
> This patch is concerned with adding a new function which will walk the free
> extents in AGFL and account for these AGFL blocks, while file system scanning.
> 
> New output (After implementing this patch).
> 
> $ uname -a
> Linux dhruv-MacBookAir 3.18.0-rc7+ #3 SMP Thu Dec 25 15:29:59 IST 2014 x86_64 x86_64 x86_64 GNU/Linux
> $ sudo xfs_spaceman -V
> xfs_spaceman version 3.2.2
> $ sudo xfs_spaceman -c "freesp" /media/xfs
>    from      to extents  blocks    pct
>       1       1      16      16   0.00
>    1024    2047       1    1262   0.04
>    4096    8191       1    5126   0.15
>    8192   16383       3   35344   1.05
>   32768   65535       1   43989   1.31
>  262144  524287       3 1334894  39.78
>  524288  967428       2 1934840  57.66

to here, this is a good patch description. :)

> Please do comment.

No need to ask in each patch description - the series description if
the place for that....

> +
> +/*
> + * Walk the free extents in the AGFL (AG Free List) of each AG, and dump

No need to explain what AGFL means - anyone reading the code already
knows....

> + * them all into the fieinfo.
> + * 
> + * With a freshly made filesystem, 4 blocks are reserved immediately after
> + * the free space B+tree root blocks (blocks 4 to 7). As they are used up

The number of blocks is actually dependent on filesystem geometry,
but again there isn't a need to describe the exact workings of the
AGFL here.

> + * additional blocks are reserved from the AG and added to the free list
> + * array. A typical device will have space for 128 elements in the array.
> + * The actual size can be determined using XFS_AGFL_SIZE macro. The array
> + * is maintained as a circular list and active elements are pointed by 
> + * AGF's agf_flfirst, agf_fllast and agf_flcount values.

Yup, you're describing the code, not /why/ the function exists.

/*
 * When we map free space we need to take into account the blocks
 * that are indexed by the AGFL. There aren't found by walking the
 * free space btrees, so we have to walk each AGFL to find them.
 */

> + */
> +static int
> +xfs_alloc_agfl_freespace_map(
> +	struct xfs_buf		**agbp,	/* buffer for a.g. freelist header */
> +	struct xfs_btree_cur    *cur,
> +	struct fiemap_extent_info *fieinfo,
> +	xfs_agblock_t		sagbno,
> +	xfs_agblock_t		eagbno)

No need to pass the freespace btree cursor into this function,
nor the agf buffer. What is required is the struct xfs_mount,
the current agno and the agf structure. i.e.

xfs_alloc_agfl_freespace_map(
	struct xfs_mount	*mp,
	struct xfs_agf		*agf,
	struct fiemap_extent_info *fieinfo,
	xfs_agnumber_t		agno,
	xfs_agblock_t		sagbno,
	xfs_agblock_t		eagbno)

> +{
> +	xfs_agf_t	*agf;		/* a.g. freespace structure */
> +	xfs_buf_t	*agflbp;	/* buffer for a.g. freelist structure */
> +	__be32		*agfl_bno;	/* Reference to freelist block */
> +	int	 	j;
                ^
> +	int 		index;
           ^
> +	int 		error = 0;
           ^

Stray whitespace.

> +
> +	agf = XFS_BUF_TO_AGF(*agbp);
> +	error = xfs_alloc_read_agfl(cur->bc_mp, NULL, be32_to_cpu(agf->agf_seqno),
> +				    &agflbp);

	error = xfs_alloc_read_agfl(mp, NULL, agno, &agflbp);

> +	if (error)
> +		return error;
> +
> +	agfl_bno = XFS_BUF_TO_AGFL_BNO(cur->bc_mp, agflbp);
> +	
> +	index = be32_to_cpu(agf->agf_flfirst);
> +	
> +	for(j=1 ; j<=be32_to_cpu(agf->agf_flcount); j++)
> +        {

formatting, and no need for j and index parameters.

	for (i = be32_to_cpu(agf->agf_flfirst);
	     i < be32_to_cpu(agf->agf_fllast);) {
	....
		if (++i == XFS_AGFL_SIZE(mp))
			i = 0;
	}

> +	 xfs_agblock_t	fbno;
> +	 xfs_extlen_t	flen;
> +	 xfs_daddr_t	dbno;
> +	 xfs_fileoff_t	dlen;	
> +	 int		flags = 0;

all the indenting in the function needs fixing.

> +
> +	/* Relative AG block number */
> +	 fbno = be32_to_cpu(agfl_bno[index]);
> +	/* Each entry in the AGFL is a single entry i.e length is 1 block */ 
> +	 flen = 1;

These can all be folded into the code below - no need for
intermediate variables...

> +
> +	/*
> +	 * AGFL is maintained as a circular list. Following is needed
> +	 * to handle array wrapping correctly.
> +	 */	
> +         if( index == ( (XFS_AGFL_SIZE(cur->bc_mp))-1 ) )
> +	  index = 0;	/* First index of AGFL */
> +         else
> +          index++;	/* Next index in AGFL */

This is handled by the above loop construct so can be ignored.
> +        
> +	/*
> +	 * Use daddr format for all range/len calculations as that is
> +	 * the format the range/len variables are supplied in by
> +	 * userspace.
> +	 */
> +	 dbno = XFS_AGB_TO_DADDR(cur->bc_mp, cur->bc_private.a.agno, fbno);
> +	 dlen = XFS_FSB_TO_BB(cur->bc_mp, flen);
> +	 error = -fiemap_fill_next_extent(fieinfo, BBTOB(dbno),
> +			                  BBTOB(dbno), BBTOB(dlen), flags);

This takes bytes as it's values, and we have AG block numbers and
filesystem block lengths. so:


and the entire loop becomes:

	for (i = be32_to_cpu(agf->agf_flfirst);
	     i < be32_to_cpu(agf->agf_fllast);) {
		/*
		 * Use daddr format for all range/len calculations as that is
		 * the format the range/len variables are supplied in by
		 * userspace.
		 */
		dbno = XFS_AGB_TO_DADDR(mp, agno, be32_to_cpu(agfl_bno[i]));
		error = fiemap_fill_next_extent(fieinfo,
						BBTOB(dbno), BBTOB(dbno),
						XFS_FSB_TO_B(mp, 1), flags);
		if (error)
			break;
		if (++i == XFS_AGFL_SIZE(mp))
			i = 0;
	}
	xfs_buf_relse(agflbp);
	return error;
}

Hmmm - I think something is still missing - what are the sagbno and
eagbno parameters supposed to do?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs_spaceman: Accounting for AGFL blocks

[ADRS PICT]

[-- Attachment #1.1: Type: text/plain, Size: 8529 bytes --]

On Wed, Jan 28, 2015 at 7:27 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Fri, Jan 23, 2015 at 09:04:00AM +0530, Dhruvesh Rathore wrote:
>> Here is the original discussion that Dave wrote for xfs_spaceman:
>>
>> http://oss.sgi.com/archives/xfs/2012-10/msg00363.html
>>
>> These patches were not posted and were just forward ported to a current
>> 3.18-rc7+ for-next XFS tree and were pushed to the fiemapfs branch in
Dave's
>> kernel tree at:
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/dgc/linux-xfs.git
>>
>> The original xfs_spaceman tool that Dave wrote is here:
>>
>> http://oss.sgi.com/archives/xfs/2012-10/msg00366.html
>>
>> Dave just updated it to the 3.2.2 code base and pushed it to the
>> spaceman branch in this tree:
>>
>> git://git.kernel.org/pub/scm/fs/xfs/xfsprogs-dev.git
>
> As per last patch, this is series description so it doesn't need to
> be repeated in every patch ;)
>

We will keep that in mind, while sending the updated patches. :)

> From here:
>
>> This xfs_spaceman utility previously failed to account for AGFL blocks.
>> Old output (Before changes).
>>
>> $ sudo xfs_spaceman -c "freesp" /media/xfs
>>    from      to extents  blocks    pct
>>    1024    2047       1    1262   0.04
>>    4096    8191       1    5126   0.15
>>    8192   16383       3   35344   1.05
>>   32768   65535       1   43989   1.31
>>  262144  524287       3 1334894  39.78
>>  524288  967428       2 1934840  57.66
>>
>> As you can see the AGFL blocks were unaccounted (4 per AG, and there
>> were 4 AGs in this filesystem).
>>
>> This patch is concerned with adding a new function which will walk the
free
>> extents in AGFL and account for these AGFL blocks, while file system
scanning.
>>
>> New output (After implementing this patch).
>>
>> $ uname -a
>> Linux dhruv-MacBookAir 3.18.0-rc7+ #3 SMP Thu Dec 25 15:29:59 IST 2014
x86_64 x86_64 x86_64 GNU/Linux
>> $ sudo xfs_spaceman -V
>> xfs_spaceman version 3.2.2
>> $ sudo xfs_spaceman -c "freesp" /media/xfs
>>    from      to extents  blocks    pct
>>       1       1      16      16   0.00
>>    1024    2047       1    1262   0.04
>>    4096    8191       1    5126   0.15
>>    8192   16383       3   35344   1.05
>>   32768   65535       1   43989   1.31
>>  262144  524287       3 1334894  39.78
>>  524288  967428       2 1934840  57.66
>
> to here, this is a good patch description. :)
>
>> Please do comment.
>
> No need to ask in each patch description - the series description if
> the place for that....
>

We will make a not of this too.

>> +
>> +/*
>> + * Walk the free extents in the AGFL (AG Free List) of each AG, and dump
>
> No need to explain what AGFL means - anyone reading the code already
> knows....
>
>> + * them all into the fieinfo.
>> + *
>> + * With a freshly made filesystem, 4 blocks are reserved immediately
after
>> + * the free space B+tree root blocks (blocks 4 to 7). As they are used
up
>
> The number of blocks is actually dependent on filesystem geometry,
> but again there isn't a need to describe the exact workings of the
> AGFL here.
>
>> + * additional blocks are reserved from the AG and added to the free list
>> + * array. A typical device will have space for 128 elements in the
array.
>> + * The actual size can be determined using XFS_AGFL_SIZE macro. The
array
>> + * is maintained as a circular list and active elements are pointed by
>> + * AGF's agf_flfirst, agf_fllast and agf_flcount values.
>
> Yup, you're describing the code, not /why/ the function exists.

Our bad, we will ensure that the comment, explains why the function is
needed.

>
> /*
>  * When we map free space we need to take into account the blocks
>  * that are indexed by the AGFL. There aren't found by walking the
>  * free space btrees, so we have to walk each AGFL to find them.
>  */
>
>> + */
>> +static int
>> +xfs_alloc_agfl_freespace_map(
>> +     struct xfs_buf          **agbp, /* buffer for a.g. freelist header
*/
>> +     struct xfs_btree_cur    *cur,
>> +     struct fiemap_extent_info *fieinfo,
>> +     xfs_agblock_t           sagbno,
>> +     xfs_agblock_t           eagbno)
>
> No need to pass the freespace btree cursor into this function,
> nor the agf buffer. What is required is the struct xfs_mount,
> the current agno and the agf structure. i.e.
> xfs_alloc_agfl_freespace_map(
>         struct xfs_mount        *mp,
>         struct xfs_agf          *agf,
>         struct fiemap_extent_info *fieinfo,
>         xfs_agnumber_t          agno,
>         xfs_agblock_t           sagbno,
>         xfs_agblock_t           eagbno)
>
>> +{
>> +     xfs_agf_t       *agf;           /* a.g. freespace structure */
>> +     xfs_buf_t       *agflbp;        /* buffer for a.g. freelist
structure */
>> +     __be32          *agfl_bno;      /* Reference to freelist block */
>> +     int             j;
>                 ^
>> +     int             index;
>            ^
>> +     int             error = 0;
>            ^
>
> Stray whitespace.
>
>> +
>> +     agf = XFS_BUF_TO_AGF(*agbp);
>> +     error = xfs_alloc_read_agfl(cur->bc_mp, NULL,
be32_to_cpu(agf->agf_seqno),
>> +                                 &agflbp);
>
>         error = xfs_alloc_read_agfl(mp, NULL, agno, &agflbp);
>
>> +     if (error)
>> +             return error;
>> +
>> +     agfl_bno = XFS_BUF_TO_AGFL_BNO(cur->bc_mp, agflbp);
>> +
>> +     index = be32_to_cpu(agf->agf_flfirst);
>> +
>> +     for(j=1 ; j<=be32_to_cpu(agf->agf_flcount); j++)
>> +        {
>
> formatting, and no need for j and index parameters.
>
>         for (i = be32_to_cpu(agf->agf_flfirst);
>              i < be32_to_cpu(agf->agf_fllast);) {
>         ....
>                 if (++i == XFS_AGFL_SIZE(mp))
>                         i = 0;
>         }
>
>> +      xfs_agblock_t  fbno;
>> +      xfs_extlen_t   flen;
>> +      xfs_daddr_t    dbno;
>> +      xfs_fileoff_t  dlen;
>> +      int            flags = 0;
>
> all the indenting in the function needs fixing.
>
>> +
>> +     /* Relative AG block number */
>> +      fbno = be32_to_cpu(agfl_bno[index]);
>> +     /* Each entry in the AGFL is a single entry i.e length is 1 block
*/
>> +      flen = 1;
>
> These can all be folded into the code below - no need for
> intermediate variables...
>
>> +
>> +     /*
>> +      * AGFL is maintained as a circular list. Following is needed
>> +      * to handle array wrapping correctly.
>> +      */
>> +         if( index == ( (XFS_AGFL_SIZE(cur->bc_mp))-1 ) )
>> +       index = 0;    /* First index of AGFL */
>> +         else
>> +          index++;   /* Next index in AGFL */
>
> This is handled by the above loop construct so can be ignored.
>> +
>> +     /*
>> +      * Use daddr format for all range/len calculations as that is
>> +      * the format the range/len variables are supplied in by
>> +      * userspace.
>> +      */
>> +      dbno = XFS_AGB_TO_DADDR(cur->bc_mp, cur->bc_private.a.agno, fbno);
>> +      dlen = XFS_FSB_TO_BB(cur->bc_mp, flen);
>> +      error = -fiemap_fill_next_extent(fieinfo, BBTOB(dbno),
>> +                                       BBTOB(dbno), BBTOB(dlen), flags);
>
> This takes bytes as it's values, and we have AG block numbers and
> filesystem block lengths. so:
>
>
> and the entire loop becomes:
>
>         for (i = be32_to_cpu(agf->agf_flfirst);
>              i < be32_to_cpu(agf->agf_fllast);) {
>                 /*
>                  * Use daddr format for all range/len calculations as
that is
>                  * the format the range/len variables are supplied in by
>                  * userspace.
>                  */
>                 dbno = XFS_AGB_TO_DADDR(mp, agno,
be32_to_cpu(agfl_bno[i]));
>                 error = fiemap_fill_next_extent(fieinfo,
>                                                 BBTOB(dbno), BBTOB(dbno),
>                                                 XFS_FSB_TO_B(mp, 1),
flags);
>                 if (error)
>                         break;
>                 if (++i == XFS_AGFL_SIZE(mp))
>                         i = 0;
>         }
>         xfs_buf_relse(agflbp);
>         return error;
> }

We will incorporate the above mentioned changes, and send the updated
patch at the earliest.

> Hmmm - I think something is still missing - what are the sagbno and
> eagbno parameters supposed to do?

Actually the parameters sagbno and eagbno are not needed in this
function and can be excluded.

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

Thank you for the detailed feedback. We will be sending the updated
patches at the earliest.

Regards,
A-DRS

[-- Attachment #1.2: Type: text/html, Size: 11640 bytes --]

[-- Attachment #2: Type: text/plain, Size: 121 bytes --]

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs_spaceman: Accounting for AGFL blocks

[Dave Chinner]

On Wed, Jan 28, 2015 at 06:35:22PM +0530, ADRS PICT wrote:
> On Wed, Jan 28, 2015 at 7:27 AM, Dave Chinner <david@fromorbit.com> wrote:
> > Hmmm - I think something is still missing - what are the sagbno and
> > eagbno parameters supposed to do?
> 
> Actually the parameters sagbno and eagbno are not needed in this
> function and can be excluded.

Why? Don't we still have to check the blocks found on the AGFL fll
within the range requested by the user, like we do for every extent
found in the btree?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Fwd: [PATCH 2/2] xfs_spaceman: Accounting for AGFL blocks

[Dhruvesh Rathore]

On Thu, Jan 29, 2015 at 2:29 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Wed, Jan 28, 2015 at 06:35:22PM +0530, ADRS PICT wrote:
>> On Wed, Jan 28, 2015 at 7:27 AM, Dave Chinner <david@fromorbit.com> wrote:
>> > Hmmm - I think something is still missing - what are the sagbno and
>> > eagbno parameters supposed to do?
>>
>> Actually the parameters sagbno and eagbno are not needed in this
>> function and can be excluded.
>
> Why? Don't we still have to check the blocks found on the AGFL fll
> within the range requested by the user, like we do for every extent
> found in the btree?

We had felt that the range check is not needed as we are fetching block numbers
from the allocation group free block array by the function
xfs_alloc_read_agfl().
And in xfs_alloc_read_agfl(), the error checking is done implicitly.

However, after you have raised this point, it is clear to us that performing a
range check will be a good way to catch and display a warning if the blocks are
out of range.

Regards,
A-DRS

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs_spaceman: Accounting for AGFL blocks

[Dhruvesh Rathore]

On Thu, Jan 29, 2015 at 9:18 AM, Dhruvesh Rathore <adrscube@gmail.com> wrote:
> On Thu, Jan 29, 2015 at 2:29 AM, Dave Chinner <david@fromorbit.com> wrote:
>> On Wed, Jan 28, 2015 at 06:35:22PM +0530, ADRS PICT wrote:
>>> On Wed, Jan 28, 2015 at 7:27 AM, Dave Chinner <david@fromorbit.com> wrote:
>>> > Hmmm - I think something is still missing - what are the sagbno and
>>> > eagbno parameters supposed to do?
>>>
>>> Actually the parameters sagbno and eagbno are not needed in this
>>> function and can be excluded.
>>
>> Why? Don't we still have to check the blocks found on the AGFL fll
>> within the range requested by the user, like we do for every extent
>> found in the btree?
>
> We had felt that the range check is not needed as we are fetching block numbers
> from the allocation group free block array by the function
> xfs_alloc_read_agfl().
> And in xfs_alloc_read_agfl(), the error checking is done implicitly.
>
> However, after you have raised this point, it is clear to us that performing a
> range check will be a good way to catch and display a warning if the blocks are
> out of range.
>

We have assimilated the changes you pointed out and have sent the
updated patches
in a new thread.

Regards,
A-DRS

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs_spaceman: Accounting for AGFL blocks

[Dhruvesh Rathore]

On Fri, Jan 30, 2015 at 7:04 AM, Dave Chinner <david@fromorbit.com> wrote:
> [ please reply to the list! ]

We apologize it was an honest mistake.
We had hit reply instead of typing in the 'to field'.

>
> On Thu, Jan 29, 2015 at 09:18:03AM +0530, Dhruvesh Rathore wrote:
>> On Thu, Jan 29, 2015 at 2:29 AM, Dave Chinner <david@fromorbit.com> wrote:
>> > On Wed, Jan 28, 2015 at 06:35:22PM +0530, ADRS PICT wrote:
>> >> On Wed, Jan 28, 2015 at 7:27 AM, Dave Chinner <david@fromorbit.com> wrote:
>> >> > Hmmm - I think something is still missing - what are the sagbno and
>> >> > eagbno parameters supposed to do?
>> >>
>> >> Actually the parameters sagbno and eagbno are not needed in this
>> >> function and can be excluded.
>> >
>> > Why? Don't we still have to check the blocks found on the AGFL fll
>> > within the range requested by the user, like we do for every extent
>> > found in the btree?
>>
>> We had felt that the range check is not needed as we are fetching block numbers
>> from the allocation group free block array by the function
>> xfs_alloc_read_agfl().
>> And in xfs_alloc_read_agfl(), the error checking is done implicitly.
>>
>> However, after you have raised this point, it is clear to us that performing a
>> range check will be a good way to catch and display a warning if the blocks are
>> out of range.

> The range checking is not for validity of the blocks themselves -
> the ranges passed in come from the user. i.e. the user is asking for
> free space blocks within a certain range on disk. The blocks in the
> AGFL may or may not lie within that the user specified range, and so
> need to be checked against the range (similar to btree extents) to
> determine if they shoul dbe added to the FIEMAPFS output...
>

We have incorporated the above changes, and sent the updated patches on
the mailing list. Here are the links:

http://oss.sgi.com/archives/xfs/2015-01/msg00484.html
http://oss.sgi.com/archives/xfs/2015-01/msg00486.html
http://oss.sgi.com/archives/xfs/2015-01/msg00487.html
http://oss.sgi.com/archives/xfs/2015-01/msg00488.html
http://oss.sgi.com/archives/xfs/2015-01/msg00489.html

Regards,
A-DRS

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs_spaceman: Accounting for AGFL blocks

[Dhruvesh Rathore]

Hi Dave,

On Fri, Jan 30, 2015 at 1:55 PM, Dhruvesh Rathore <adrscube@gmail.com> wrote:
> http://oss.sgi.com/archives/xfs/2015-01/msg00484.html
> http://oss.sgi.com/archives/xfs/2015-01/msg00486.html
> http://oss.sgi.com/archives/xfs/2015-01/msg00487.html
> http://oss.sgi.com/archives/xfs/2015-01/msg00488.html
> http://oss.sgi.com/archives/xfs/2015-01/msg00489.html

We were wondering if you had a chance to review the above sent patches. :)

Regards,
A-DRS

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs