Re: review: fsblock zero - don't panic

[Stewart Smith <stewart@flamingspork.com>]

[-- Attachment #1: Type: text/plain, Size: 4880 bytes --]

On Wed, 2006-08-16 at 14:28 +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?
> 
> cheers.
> 
> -- 
> Nathan
> 
> 
> Index: xfs-linux/xfs_bmap.c
> ===================================================================
> --- xfs-linux.orig/xfs_bmap.c	2006-08-15 15:22:53.198187750 +1000
> +++ xfs-linux/xfs_bmap.c	2006-08-16 12:40:49.669518000 +1000
> @@ -3705,7 +3705,7 @@ STATIC xfs_bmbt_rec_t *                 
>  xfs_bmap_search_extents(
>  	xfs_inode_t     *ip,            /* incore inode pointer */
>  	xfs_fileoff_t   bno,            /* block number searched for */
> -	int             whichfork,      /* data or attr fork */
> +	int             fork,      	/* data or attr fork */
>  	int             *eofp,          /* out: end of file found */
>  	xfs_extnum_t    *lastxp,        /* out: last extent index */
>  	xfs_bmbt_irec_t *gotp,          /* out: extent entry found */
> @@ -3713,25 +3713,28 @@ xfs_bmap_search_extents(
>  {
>  	xfs_ifork_t	*ifp;		/* inode fork pointer */
>  	xfs_bmbt_rec_t  *ep;            /* extent record pointer */
> -	int		rt;		/* realtime flag    */
>  
>  	XFS_STATS_INC(xs_look_exlist);
> -	ifp = XFS_IFORK_PTR(ip, whichfork);
> +	ifp = XFS_IFORK_PTR(ip, fork);
>  
>  	ep = xfs_bmap_search_multi_extents(ifp, bno, eofp, lastxp, gotp, prevp);
>  
> -	rt = (whichfork == XFS_DATA_FORK) && XFS_IS_REALTIME_INODE(ip);
> -	if (unlikely(!rt && !gotp->br_startblock && (*lastxp != NULLEXTNUM))) {
> -                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,
> +	if (unlikely(!(gotp->br_startblock) && (*lastxp != NULLEXTNUM) &&
> +		     !(XFS_IS_REALTIME_INODE(ip) && fork == XFS_DATA_FORK))) {
> +		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 lastx: %x\n",
> +			(unsigned long long)ip->i_ino,
>  			(unsigned long long)gotp->br_startblock,
>  			(unsigned long long)gotp->br_startoff,
>  			(unsigned long long)gotp->br_blockcount,
> -			gotp->br_state);
> -        }
> -        return ep;
> +			gotp->br_state, *lastxp);
> +		*lastxp = NULLEXTNUM;
> +		*eofp = 1;
> +		return NULL;
> +	}
> +	return ep;
>  }
>  
> 
> 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) {
> -                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 &&
> +		     !(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);
> -        }
> +		error = EFSCORRUPTED;

should this be XFS_ERROR(EFSCORRUPTED) ?

perhaps change to use it for consistency if the conversion is done in
error_out?

> +		goto error_out;

-- 
Stewart Smith (stewart@flamingspork.com)
http://www.flamingspork.com/


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 191 bytes --]