xfs
[Top] [All Lists]

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

To: Alex Elder <aelder@xxxxxxx>
Subject: Re: [PATCH 2/10] xfs: fix min bufsize bugs in two places
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Mon, 12 Apr 2010 16:32:48 +1000
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1270852013.7840.132.camel@doink>
References: <1270852013.7840.132.camel@doink>
User-agent: Mutt/1.5.20 (2009-06-14)
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@xxxxxxx>
> 
> ---
> 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@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>