Re: [PATCH] xfs: skip verification on initial "guess" superblock read

[Eric Sandeen <sandeen@redhat.com>]

On 2/13/14, 4:08 PM, Dave Chinner wrote:
> On Thu, Feb 13, 2014 at 03:42:49PM -0600, Eric Sandeen wrote:
>> When xfs_readsb() does the very first read of the superblock,
>> it makes a guess at the length of the buffer, based on the
>> sector size of the underlying storage.  This may or may
>> not match the filesystem sector size in sb_sectsize, so
>> we can't i.e. do a CRC check on it; it might be too short.
>>
>> In fact, mounting a filesystem with sb_sectsize larger
>> than the device sector size will cause a mount failure
>> if CRCs are enabled, because we are checksumming a length
>> which exceeds the buffer passed to it.
>>
>> The only really foolproof way I see around this is to
>> *always* read the superblock twice in xfs_readsb();
>> first with a guess, and again with a length as indicated
>> by the superblock's sb_sectsize.  The verifier for
>> this guessed length buffer is a no-op; we'll do proper
>> verification on the 2nd time around.
> 
> We don't need a verifier for a "don't verify" read. Just pass in
> NULL to avoid verification completely.

Oh!

        if (read && bp->b_ops && !bp->b_error && (bp->b_flags & XBF_DONE))
                bp->b_ops->verify_read(bp);

Doh, ok, yeah that is simpler.  Didn't think about that.  :/

> Also, you don't need to re-read the buffer to verify it unless the
> sector size was not what was predicted. All you need to do is call
> the verifier directly on the buffer in question, then attach it
> directly to the buffer. So no need for @xfs_sb_guess_buf_ops. :)

Hum, ok, let me ponder that.

-Eric

> Cheers,
> 
> Dave.
> 
> 
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>  
>> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
>> index 02df7b4..b4413fe 100644
>> --- a/fs/xfs/xfs_mount.c
>> +++ b/fs/xfs/xfs_mount.c
>> @@ -282,22 +282,28 @@ xfs_readsb(
>>  	struct xfs_sb	*sbp = &mp->m_sb;
>>  	int		error;
>>  	int		loud = !(flags & XFS_MFSI_QUIET);
>> +	const struct xfs_buf_ops *buf_ops;
>>  
>>  	ASSERT(mp->m_sb_bp == NULL);
>>  	ASSERT(mp->m_ddev_targp != NULL);
>>  
>>  	/*
>> +	 * For the initial read, we must guess at the sector
>> +	 * size based on the block device.  It's enough to
>> +	 * get the sb_sectsize out of the superblock and
>> +	 * then reread with the proper length.
>> +	 */
>> +	sector_size = xfs_getsize_buftarg(mp->m_ddev_targp);
>> +	buf_ops = &xfs_sb_guess_buf_ops;
>> +
>> +	/*
>>  	 * Allocate a (locked) buffer to hold the superblock.
>>  	 * This will be kept around at all times to optimize
>>  	 * access to the superblock.
>>  	 */
>> -	sector_size = xfs_getsize_buftarg(mp->m_ddev_targp);
>> -
>>  reread:
>>  	bp = xfs_buf_read_uncached(mp->m_ddev_targp, XFS_SB_DADDR,
>> -				   BTOBB(sector_size), 0,
>> -				   loud ? &xfs_sb_buf_ops
>> -				        : &xfs_sb_quiet_buf_ops);
>> +				   BTOBB(sector_size), 0, buf_ops);
>>  	if (!bp) {
>>  		if (loud)
>>  			xfs_warn(mp, "SB buffer read failed");
>> @@ -328,12 +334,13 @@ reread:
>>  	}
>>  
>>  	/*
>> -	 * If device sector size is smaller than the superblock size,
>> -	 * re-read the superblock so the buffer is correctly sized.
>> +	 * Re-read the superblock so the buffer is correctly sized,
>> +	 * and properly verified.
>>  	 */
>> -	if (sector_size < sbp->sb_sectsize) {
>> +	if (buf_ops == &xfs_sb_guess_buf_ops) {
>>  		xfs_buf_relse(bp);
>>  		sector_size = sbp->sb_sectsize;
>> +		buf_ops = loud ? &xfs_sb_buf_ops : &xfs_sb_quiet_buf_ops;
>>  		goto reread;
>>  	}
>>  
>> diff --git a/fs/xfs/xfs_sb.c b/fs/xfs/xfs_sb.c
>> index 5071ccb..abbbbb7 100644
>> --- a/fs/xfs/xfs_sb.c
>> +++ b/fs/xfs/xfs_sb.c
>> @@ -653,6 +653,20 @@ xfs_sb_quiet_read_verify(
>>  	/* quietly fail */
>>  	xfs_buf_ioerror(bp, EWRONGFS);
>>  }
>> +/*
>> + * The initial superblock read from xfs_readsb only guesses at
>> + * the sector size based on the block device sector size, so
>> + * we may get here with a buffer length shorter than the filesystem
>> + * sb_sectsize.  We can't properly verify it, so just return,
>> + * and xfs_readsb will call the proper verifer with a real length
>> + * on the 2nd time around.
>> + */
>> +static void
>> +xfs_sb_guess_read_verify(
>> +	struct xfs_buf	*bp)
>> +{
>> +	return;
>> +}
>>  
>>  static void
>>  xfs_sb_write_verify(
>> @@ -680,16 +694,24 @@ xfs_sb_write_verify(
>>  			 offsetof(struct xfs_sb, sb_crc));
>>  }
>>  
>> +/* Normal read verifier */
>>  const struct xfs_buf_ops xfs_sb_buf_ops = {
>>  	.verify_read = xfs_sb_read_verify,
>>  	.verify_write = xfs_sb_write_verify,
>>  };
>>  
>> +/* Quiet verifier for MS_SILENT mounts; ignore non-XFS magic */
>>  const struct xfs_buf_ops xfs_sb_quiet_buf_ops = {
>>  	.verify_read = xfs_sb_quiet_read_verify,
>>  	.verify_write = xfs_sb_write_verify,
>>  };
>>  
>> +/* The very first superblock read must guess at the size */
>> +const struct xfs_buf_ops xfs_sb_guess_buf_ops = {
>> +	.verify_read = xfs_sb_guess_read_verify,
>> +	.verify_write = xfs_sb_write_verify,
>> +};
>> +
>>  /*
>>   * xfs_mount_common
>>   *
>> diff --git a/fs/xfs/xfs_shared.h b/fs/xfs/xfs_shared.h
>> index 8c5035a..a4294a6 100644
>> --- a/fs/xfs/xfs_shared.h
>> +++ b/fs/xfs/xfs_shared.h
>> @@ -51,6 +51,7 @@ extern const struct xfs_buf_ops xfs_inode_buf_ra_ops;
>>  extern const struct xfs_buf_ops xfs_dquot_buf_ops;
>>  extern const struct xfs_buf_ops xfs_sb_buf_ops;
>>  extern const struct xfs_buf_ops xfs_sb_quiet_buf_ops;
>> +extern const struct xfs_buf_ops xfs_sb_guess_buf_ops;
>>  extern const struct xfs_buf_ops xfs_symlink_buf_ops;
>>  
>>  /*
>>
>> _______________________________________________
>> xfs mailing list
>> xfs@oss.sgi.com
>> http://oss.sgi.com/mailman/listinfo/xfs
>>
> 

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