[PATCH 2/10] xfs: fix min bufsize bugs in two places

[PATCH 2/10] xfs: fix min bufsize bugs in two places

[Alex Elder]

This fixes a bug in two places that I found by inspection.  In
xlog_find_verify_cycle() and xlog_write_log_records(), the code
attempts to allocate a buffer to hold as many blocks as possible.
It gives up if the number of blocks to be allocated gets too small.
Right now it uses log->l_sectbb_log as that lower bound, but I'm
sure it's supposed to be the actual log sector size instead.  That
is, the lower bound should be (1 << log->l_sectbb_log).

Also define a simple macro xlog_sectbb(log) to represent the number
of basic blocks in a sector for the given log.

(No change from original submission; I have implemented Christoph's
suggestion about storing l_sectsize rather than l_sectbb_log in
a new, separate patch in this series.)

Signed-off-by: Alex Elder <aelder@sgi.com>

---
fs/xfs/xfs_log_recover.c |   21 +++++++++++++++++----
 fs/xfs/xfs_log_recover.c |   21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

Index: b/fs/xfs/xfs_log_recover.c
===================================================================
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -66,6 +66,9 @@ STATIC void	xlog_recover_check_summary(x
 	((bbs + (log)->l_sectbb_mask + 1) & ~(log)->l_sectbb_mask) : (bbs) )
 #define XLOG_SECTOR_ROUNDDOWN_BLKNO(log, bno)	((bno) & ~(log)->l_sectbb_mask)
 
+/* Number of basic blocks in a log sector */
+#define xlog_sectbb(log) (1 << (log)->l_sectbb_log)
+
 STATIC xfs_buf_t *
 xlog_get_bp(
 	xlog_t		*log,
@@ -376,12 +379,16 @@ xlog_find_verify_cycle(
 	xfs_caddr_t	buf = NULL;
 	int		error = 0;
 
+	/*
+	 * Greedily allocate a buffer big enough to handle the full
+	 * range of basic blocks we'll be examining.  If that fails,
+	 * try a smaller size.  We need to be able to read at least
+	 * a log sector, or we're out of luck.
+	 */
 	bufblks = 1 << ffs(nbblks);
-
 	while (!(bp = xlog_get_bp(log, bufblks))) {
-		/* can't get enough memory to do everything in one big buffer */
 		bufblks >>= 1;
-		if (bufblks <= log->l_sectbb_log)
+		if (bufblks < xlog_sectbb(log))
 			return ENOMEM;
 	}
 
@@ -1158,10 +1165,16 @@ xlog_write_log_records(
 	int		error = 0;
 	int		i, j = 0;
 
+	/*
+	 * Greedily allocate a buffer big enough to handle the full
+	 * range of basic blocks to be written.  If that fails, try
+	 * a smaller size.  We need to be able to write at least a
+	 * log sector, or we're out of luck.
+	 */
 	bufblks = 1 << ffs(blocks);
 	while (!(bp = xlog_get_bp(log, bufblks))) {
 		bufblks >>= 1;
-		if (bufblks <= log->l_sectbb_log)
+		if (bufblks < xlog_sectbb(log))
 			return ENOMEM;
 	}
 


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

Re: [PATCH 2/10] xfs: fix min bufsize bugs in two places

[Dave Chinner]

On Fri, Apr 09, 2010 at 05:26:53PM -0500, Alex Elder wrote:
> This fixes a bug in two places that I found by inspection.  In
> xlog_find_verify_cycle() and xlog_write_log_records(), the code
> attempts to allocate a buffer to hold as many blocks as possible.
> It gives up if the number of blocks to be allocated gets too small.
> Right now it uses log->l_sectbb_log as that lower bound, but I'm
> sure it's supposed to be the actual log sector size instead.  That
> is, the lower bound should be (1 << log->l_sectbb_log).
> 
> Also define a simple macro xlog_sectbb(log) to represent the number
> of basic blocks in a sector for the given log.
> 
> (No change from original submission; I have implemented Christoph's
> suggestion about storing l_sectsize rather than l_sectbb_log in
> a new, separate patch in this series.)
> 
> Signed-off-by: Alex Elder <aelder@sgi.com>
> 
> ---
> fs/xfs/xfs_log_recover.c |   21 +++++++++++++++++----
>  fs/xfs/xfs_log_recover.c |   21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> Index: b/fs/xfs/xfs_log_recover.c
> ===================================================================
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -66,6 +66,9 @@ STATIC void	xlog_recover_check_summary(x
>  	((bbs + (log)->l_sectbb_mask + 1) & ~(log)->l_sectbb_mask) : (bbs) )
>  #define XLOG_SECTOR_ROUNDDOWN_BLKNO(log, bno)	((bno) & ~(log)->l_sectbb_mask)
>  
> +/* Number of basic blocks in a log sector */
> +#define xlog_sectbb(log) (1 << (log)->l_sectbb_log)
> +

Can you do this without the #define? There's a spot in
xlog_alloc_log() where the l_sectbb_mask is set up that
open code this shift, so it seems to me that this woul dbe the spot
to set up a l_sect_size variable and reference that instead...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 2/10] xfs: fix min bufsize bugs in two places

[Dave Chinner]

On Mon, Apr 12, 2010 at 04:32:48PM +1000, Dave Chinner wrote:
> On Fri, Apr 09, 2010 at 05:26:53PM -0500, Alex Elder wrote:
> > This fixes a bug in two places that I found by inspection.  In
> > xlog_find_verify_cycle() and xlog_write_log_records(), the code
> > attempts to allocate a buffer to hold as many blocks as possible.
> > It gives up if the number of blocks to be allocated gets too small.
> > Right now it uses log->l_sectbb_log as that lower bound, but I'm
> > sure it's supposed to be the actual log sector size instead.  That
> > is, the lower bound should be (1 << log->l_sectbb_log).
> > 
> > Also define a simple macro xlog_sectbb(log) to represent the number
> > of basic blocks in a sector for the given log.
> > 
> > (No change from original submission; I have implemented Christoph's
> > suggestion about storing l_sectsize rather than l_sectbb_log in
> > a new, separate patch in this series.)
> > 
> > Signed-off-by: Alex Elder <aelder@sgi.com>
> > 
> > ---
> > fs/xfs/xfs_log_recover.c |   21 +++++++++++++++++----
> >  fs/xfs/xfs_log_recover.c |   21 +++++++++++++++++----
> >  1 file changed, 17 insertions(+), 4 deletions(-)
> > 
> > Index: b/fs/xfs/xfs_log_recover.c
> > ===================================================================
> > --- a/fs/xfs/xfs_log_recover.c
> > +++ b/fs/xfs/xfs_log_recover.c
> > @@ -66,6 +66,9 @@ STATIC void	xlog_recover_check_summary(x
> >  	((bbs + (log)->l_sectbb_mask + 1) & ~(log)->l_sectbb_mask) : (bbs) )
> >  #define XLOG_SECTOR_ROUNDDOWN_BLKNO(log, bno)	((bno) & ~(log)->l_sectbb_mask)
> >  
> > +/* Number of basic blocks in a log sector */
> > +#define xlog_sectbb(log) (1 << (log)->l_sectbb_log)
> > +
> 
> Can you do this without the #define? There's a spot in
> xlog_alloc_log() where the l_sectbb_mask is set up that
> open code this shift, so it seems to me that this woul dbe the spot
> to set up a l_sect_size variable and reference that instead...

I note you did this with a later patch, so this one is good to go.

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

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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