xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfs: skip verification on initial "guess" superblock read
From: Eric Sandeen <sandeen@xxxxxxxxxx>
Date: Thu, 13 Feb 2014 16:12:07 -0600
Cc: xfs-oss <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20140213220812.GI13997@dastard>
References: <52FD3C59.3050400@xxxxxxxxxx> <20140213220812.GI13997@dastard>
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:24.0) Gecko/20100101 Thunderbird/24.3.0
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@xxxxxxxxxx>
>> ---
>>  
>> 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@xxxxxxxxxxx
>> http://oss.sgi.com/mailman/listinfo/xfs
>>
> 

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