Re: review: fsblock zero - don't panic

[David Chinner <dgc@sgi.com>]

On Wed, Aug 16, 2006 at 02:28:01PM +1000, Nathan Scott wrote:
> On Fri, Aug 11, 2006 at 01:26:26PM +1000, David Chinner wrote:
> > ...
> > Looks OK.
> 
> Thanks mate.
> 
> > ...
> > If you are hinting this branch to be unlikely, then we should also
> > check the start block first before checking the io_flags.  We only
> > need to check the io_flags if we are actually accessing block zero.
> 
> I've rolled this into a new version.  I also got prodded to still
> provide an option to panic on detecting this, for debugging - so I
> rewrote this to report through the panic_mask mechanism, which now
> gives us the option to optionally panic if need be... could you do
> a second pass over this for me?

Couple of things:

> Index: xfs-linux/xfs_iomap.c
> ===================================================================
> --- xfs-linux.orig/xfs_iomap.c	2006-08-15 15:22:53.238190250 +1000
> +++ xfs-linux/xfs_iomap.c	2006-08-16 12:40:41.385000250 +1000
> @@ -536,23 +536,27 @@ xfs_iomap_write_direct(
>  	 * Copy any maps to caller's array and return any error.
>  	 */
>  	if (nimaps == 0) {
> -		error = (ENOSPC);
> +		error = ENOSPC;
>  		goto error_out;
>  	}
>  
> -	*ret_imap = imap;
> -	*nmaps = 1;
> -	if ( !(io->io_flags & XFS_IOCORE_RT)  && !ret_imap->br_startblock) {

Here we check imap for block zero access....

> -                cmn_err(CE_PANIC,"Access to block zero:  fs <%s> inode: %lld "
> -                        "start_block : %llx start_off : %llx blkcnt : %llx "
> -                        "extent-state : %x \n",
> -                        (ip->i_mount)->m_fsname,
> -                        (long long)ip->i_ino,
> -                        (unsigned long long)ret_imap->br_startblock,
> +	if (unlikely(!ret_imap->br_startblock &&

and now you're checking ret_imap for zero block access. Shouldn't that
be checking imap?

> +		     !(io->io_flags & XFS_IOCORE_RT))) {
> +		xfs_cmn_err(XFS_PTAG_FSBLOCK_ZERO, CE_ALERT, ip->i_mount,
> +				"Access to block zero in inode %llu "
> +				"start_block: %llx start_off: %llx "
> +				"blkcnt: %llx extent-state: %x\n",
> +			(unsigned long long)ip->i_ino,
> +			(unsigned long long)ret_imap->br_startblock,
>  			(unsigned long long)ret_imap->br_startoff,
> -                        (unsigned long long)ret_imap->br_blockcount,
> +			(unsigned long long)ret_imap->br_blockcount,
>  			ret_imap->br_state);

FWIW, given the number of times this exact same code is given, a
wrapper function for it would clean up the code quite a bit. i.e.

		xfs_cmn_err_fsblock_zero(ip, ret_imap);

would replace the above mess....

> -        }
> +		error = EFSCORRUPTED;
> +		goto error_out;
> +	}
> +
> +	*ret_imap = imap;
> +	*nmaps = 1;
>  	return 0;
>  
>  error0:	/* Cancel bmap, unlock inode, unreserve quota blocks, cancel trans */
> @@ -715,16 +719,18 @@ retry:
>  		goto retry;
>  	}
>  
> -	if (!(io->io_flags & XFS_IOCORE_RT)  && !ret_imap->br_startblock) {

And this one checks ret_imap for block zero, not imap[0].

One of these original checks was buggy - which one should we be checking,
ret_imap that is being passed into the function or imap that is what was
returned  by the XFS_BMAPI call?

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group