[PATCHv2 5/5] xfs: kill off l_sectbb_mask

[PATCHv2 5/5] xfs: kill off l_sectbb_mask

[Alex Elder]

There remains only one user of the l_sectbb_mask field in the log
structure.  Just kill it off and compute the mask where needed from
the power-of-2 sector size.

(Only update from last post is to accomodate the changes in the
previous patch in the series.)

Signed-off-by: Alex Elder <aelder@sgi.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>

---
 fs/xfs/xfs_log.c         |    1 -
 fs/xfs/xfs_log_priv.h    |    4 +---
 fs/xfs/xfs_log_recover.c |   14 +++++++++-----
 3 files changed, 10 insertions(+), 9 deletions(-)

Index: b/fs/xfs/xfs_log.c
===================================================================
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -1089,7 +1089,6 @@ xlog_alloc_log(xfs_mount_t	*mp,
 		}
 	}
 	log->l_sectBBsize = 1 << log2_size;
-	log->l_sectbb_mask = log->l_sectBBsize - 1;
 
 	xlog_get_iclog_buffer_size(mp, log);
 
Index: b/fs/xfs/xfs_log_priv.h
===================================================================
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -396,9 +396,7 @@ typedef struct log {
 	struct xfs_buf_cancel	**l_buf_cancel_table;
 	int			l_iclog_hsize;  /* size of iclog header */
 	int			l_iclog_heads;  /* # of iclog header sectors */
-	uint			l_sectBBsize;   /* sector size in BBs */
-	uint			l_sectbb_mask;  /* sector size (in BBs)
-						 * alignment mask */
+	uint			l_sectBBsize;   /* sector size in BBs (2^n) */
 	int			l_iclog_size;	/* size of log in bytes */
 	int			l_iclog_size_log; /* log power size of log */
 	int			l_iclog_bufs;	/* number of iclog buffers */
Index: b/fs/xfs/xfs_log_recover.c
===================================================================
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -121,6 +121,10 @@ xlog_put_bp(
 	xfs_buf_free(bp);
 }
 
+/*
+ * Return the address of the start of the given block number's data
+ * in a log buffer.  The buffer covers a log sector-aligned region.
+ */
 STATIC xfs_caddr_t
 xlog_align(
 	xlog_t		*log,
@@ -128,14 +132,14 @@ xlog_align(
 	int		nbblks,
 	xfs_buf_t	*bp)
 {
+	xfs_daddr_t	offset;
 	xfs_caddr_t	ptr;
 
-	if (log->l_sectBBsize == 1)
-		return XFS_BUF_PTR(bp);
+	offset = blk_no & ((xfs_daddr_t) log->l_sectBBsize - 1);
+	ptr = XFS_BUF_PTR(bp) + BBTOB(offset);
+
+	ASSERT(ptr + BBTOB(nbblks) <= XFS_BUF_PTR(bp) + XFS_BUF_SIZE(bp));
 
-	ptr = XFS_BUF_PTR(bp) + BBTOB((int)blk_no & log->l_sectbb_mask);
-	ASSERT(XFS_BUF_SIZE(bp) >=
-		BBTOB(nbblks + (blk_no & log->l_sectbb_mask)));
 	return ptr;
 }
 

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

Re: [PATCHv2 5/5] xfs: kill off l_sectbb_mask

[Dave Chinner]

On Fri, Apr 16, 2010 at 03:54:05PM -0500, Alex Elder wrote:
> There remains only one user of the l_sectbb_mask field in the log
> structure.  Just kill it off and compute the mask where needed from
> the power-of-2 sector size.
> 
> (Only update from last post is to accomodate the changes in the
> previous patch in the series.)
> 
> Signed-off-by: Alex Elder <aelder@sgi.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
>  	xfs_buf_t	*bp)
>  {
> +	xfs_daddr_t	offset;
>  	xfs_caddr_t	ptr;
>  
> -	if (log->l_sectBBsize == 1)
> -		return XFS_BUF_PTR(bp);
> +	offset = blk_no & ((xfs_daddr_t) log->l_sectBBsize - 1);
> +	ptr = XFS_BUF_PTR(bp) + BBTOB(offset);
> +
> +	ASSERT(ptr + BBTOB(nbblks) <= XFS_BUF_PTR(bp) + XFS_BUF_SIZE(bp));

The ASSERT is more obfuscated than it needs to be. It's not obvious
that it is bounds checking offset+nbblks. i.e. I prefer the original
format like:

	ASSERT(BBTOB(offset + nbblks) <= XFS_BUF_SIZE(bp));

but otherwise it looks good. Anyway, minor nitpick, so:

Reviewed-by: Dave Chinner <david@fromorbit.com>

-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCHv2 5/5] xfs: kill off l_sectbb_mask

[Christoph Hellwig]

On Fri, Apr 16, 2010 at 03:54:05PM -0500, Alex Elder wrote:
> -	uint			l_sectBBsize;   /* sector size in BBs */
> -	uint			l_sectbb_mask;  /* sector size (in BBs)
> -						 * alignment mask */
> +	uint			l_sectBBsize;   /* sector size in BBs (2^n) */

The 2^n comment doesn't make too much sense to me.

But in general looks good, nice cleanup.


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

> @@ -128,14 +132,14 @@ xlog_align(
>  	int		nbblks,
>  	xfs_buf_t	*bp)
>  {
> +	xfs_daddr_t	offset;
>  	xfs_caddr_t	ptr;
>  
> -	if (log->l_sectBBsize == 1)
> -		return XFS_BUF_PTR(bp);
> +	offset = blk_no & ((xfs_daddr_t) log->l_sectBBsize - 1);
> +	ptr = XFS_BUF_PTR(bp) + BBTOB(offset);
> +
> +	ASSERT(ptr + BBTOB(nbblks) <= XFS_BUF_PTR(bp) + XFS_BUF_SIZE(bp));

And Dave's version of this assert definitively makes a lot more sense,
this is one while evolved from the previous one is rather ugly.

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

Re: [PATCHv2 5/5] xfs: kill off l_sectbb_mask

[Christoph Hellwig]

On Fri, Apr 16, 2010 at 03:54:05PM -0500, Alex Elder wrote:
> + * Return the address of the start of the given block number's data
> + * in a log buffer.  The buffer covers a log sector-aligned region.
> + */
>  STATIC xfs_caddr_t
>  xlog_align(
>  	xlog_t		*log,
> @@ -128,14 +132,14 @@ xlog_align(
>  	int		nbblks,
>  	xfs_buf_t	*bp)
>  {
> +	xfs_daddr_t	offset;
>  	xfs_caddr_t	ptr;
>  
> -	if (log->l_sectBBsize == 1)
> -		return XFS_BUF_PTR(bp);
> +	offset = blk_no & ((xfs_daddr_t) log->l_sectBBsize - 1);
> +	ptr = XFS_BUF_PTR(bp) + BBTOB(offset);
> +
> +	ASSERT(ptr + BBTOB(nbblks) <= XFS_BUF_PTR(bp) + XFS_BUF_SIZE(bp));
>  
> -	ptr = XFS_BUF_PTR(bp) + BBTOB((int)blk_no & log->l_sectbb_mask);
> -	ASSERT(XFS_BUF_SIZE(bp) >=
> -		BBTOB(nbblks + (blk_no & log->l_sectbb_mask)));
>  	return ptr;

And btw, now that I think about it we can just remove the
log->l_sectBBsize == 1 case entirely, so the whole thing could become:

STATIC xfs_caddr_t
xlog_align(
	struct log	*log,
	xfs_daddr_t	blk_no,
	int		nbblks,
	struct xfs_buf	*bp)
{
	xfs_daddr_t	offset;

	offset = blk_no & ((xfs_daddr_t) log->l_sectBBsize - 1);
	ASSERT(BBTOB(offset + nbblks) <= XFS_BUF_SIZE(bp));

	return XFS_BUF_PTR(bp) + BBTOB(offset);
}

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

Re: [PATCHv2 5/5] xfs: kill off l_sectbb_mask

[Alex Elder]

On Sun, 2010-04-18 at 13:29 -0400, Christoph Hellwig wrote:
> On Fri, Apr 16, 2010 at 03:54:05PM -0500, Alex Elder wrote:
> > + * Return the address of the start of the given block number's data
> > + * in a log buffer.  The buffer covers a log sector-aligned region.
> > + */
> >  STATIC xfs_caddr_t
> >  xlog_align(
> >  	xlog_t		*log,
> > @@ -128,14 +132,14 @@ xlog_align(
> >  	int		nbblks,
> >  	xfs_buf_t	*bp)
> >  {
> > +	xfs_daddr_t	offset;
> >  	xfs_caddr_t	ptr;
> >  
> > -	if (log->l_sectBBsize == 1)
> > -		return XFS_BUF_PTR(bp);
> > +	offset = blk_no & ((xfs_daddr_t) log->l_sectBBsize - 1);
> > +	ptr = XFS_BUF_PTR(bp) + BBTOB(offset);
> > +
> > +	ASSERT(ptr + BBTOB(nbblks) <= XFS_BUF_PTR(bp) + XFS_BUF_SIZE(bp));
> >  
> > -	ptr = XFS_BUF_PTR(bp) + BBTOB((int)blk_no & log->l_sectbb_mask);
> > -	ASSERT(XFS_BUF_SIZE(bp) >=
> > -		BBTOB(nbblks + (blk_no & log->l_sectbb_mask)));
> >  	return ptr;
> 
> And btw, now that I think about it we can just remove the
> log->l_sectBBsize == 1 case entirely, so the whole thing could become:

Yes, I contemplated that and am happy to do it that way.
I chose this way because there seemed to be "so much" cruft
that was not needed in that special case...  Anyway, I'll
change it to the way you suggest before committing it.

Thanks for the review.

					-Alex

> STATIC xfs_caddr_t
> xlog_align(
> 	struct log	*log,
> 	xfs_daddr_t	blk_no,
> 	int		nbblks,
> 	struct xfs_buf	*bp)
> {
> 	xfs_daddr_t	offset;
> 
> 	offset = blk_no & ((xfs_daddr_t) log->l_sectBBsize - 1);
> 	ASSERT(BBTOB(offset + nbblks) <= XFS_BUF_SIZE(bp));
> 
> 	return XFS_BUF_PTR(bp) + BBTOB(offset);
> }



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