svn commit: r226552 - head/sys/boot/zfs

[Andriy Gapon]

on 20/10/2011 02:40 Pawel Jakub Dawidek said the following:
> Author: pjd
> Date: Wed Oct 19 23:40:37 2011
> New Revision: 226552
> URL: http://svn.freebsd.org/changeset/base/226552
> 
> Log:
>   Never pass NULL block pointer when reading. This is neither expected nor
>   handled by lower layers like vdev_raidz, which uses bp for checksum
>   verification. This bug could lead to NULL pointer reference and resets
>   during boot.
>   
>   MFC after:	3 days
> 
> Modified:
>   head/sys/boot/zfs/zfsimpl.c
> 
> Modified: head/sys/boot/zfs/zfsimpl.c
> ==============================================================================
> --- head/sys/boot/zfs/zfsimpl.c	Wed Oct 19 23:37:30 2011	(r226551)
> +++ head/sys/boot/zfs/zfsimpl.c	Wed Oct 19 23:40:37 2011	(r226552)
> @@ -988,7 +988,8 @@ zio_read_gang(spa_t *spa, const blkptr_t
>  			break;
>  	if (!vdev || !vdev->v_read)
>  		return (EIO);
> -	if (vdev->v_read(vdev, NULL, &zio_gb, offset, SPA_GANGBLOCKSIZE))
> +
> +	if (vdev->v_read(vdev, bp, &zio_gb, offset, SPA_GANGBLOCKSIZE))
>  		return (EIO);
>  
>  	for (i = 0; i < SPA_GBH_NBLKPTRS; i++) {


I believe that this commit is incorrect.
It does appear to do the right thing for raidz, but I believe that it breaks the
simple case when v_read is vdev_read_phys.  Now because the bp argument is not
NULL, the latter will try to read BP_GET_PSIZE(bp) bytes instead of
SPA_GANGBLOCKSIZE bytes.

I believe that my patch that I shared (much) earlier handles this issue
correctly and integrally:
http://article.gmane.org/gmane.os.freebsd.devel.file-systems/13130
Did you find anything wrong with it?
If not, then maybe we can get back to discussing it?

-- 
Andriy Gapon