Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1718612 > unrolled thread

Re: Kernels v4.9+ cause short reads of block devices

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: Kernels v4.9+ cause short reads of block devices Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-23 21:40 +0200
    Re: Kernels v4.9+ cause short reads of block devices Doug Nazar <nazard@nazar.ca> - 2017-08-23 22:00 +0200
      Re: Kernels v4.9+ cause short reads of block devices Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-23 22:20 +0200
        Re: Kernels v4.9+ cause short reads of block devices Andreas Dilger <adilger@dilger.ca> - 2017-08-23 23:10 +0200
          Re: Kernels v4.9+ cause short reads of block devices Doug Nazar <nazard@nazar.ca> - 2017-08-24 12:30 +0200
            Re: Kernels v4.9+ cause short reads of block devices Dave Kleikamp <dave.kleikamp@oracle.com> - 2017-08-24 17:30 +0200
        Re: Kernels v4.9+ cause short reads of block devices Doug Nazar <nazard@nazar.ca> - 2017-08-24 12:10 +0200

#1718612 — Re: Kernels v4.9+ cause short reads of block devices

[Multipart message — attachments visible in raw view] — view raw

On Wed, Aug 23, 2017 at 12:15 PM, Doug Nazar <nazard@nazar.ca> wrote:
> The following commits cause short reads of block devices, however writes are
> still allowed.
>
> c2a9737f45e2 ("vfs,mm: fix a dead loop in truncate_inode_pages_range()")
> d05c5f7ba164 ("vfs,mm: fix return value of read() at s_maxbytes")
>
> When e2fsck sees this, it thinks it's a bad sector and tries to write a
> block of nulls which overwrites the valid data.

Hmm. Block devices shouldn't have issues with s_maxbytes, and I'm
surprised that nobody has seen that before.

> Device is LVM over 2 x RAID-5 on an old 32bit desktop.
>
> RO    RA   SSZ   BSZ   StartSec            Size   Device
> rw  4096   512  4096          0   9748044840960 /dev/Storage/Main

.. and the problem may be as simple as just a missing initialization
of s_maxbytes for blockdev_superblock.

Does the attcahed trivial one-liner fix things for you?

Al, if it really is this simple, how come nobody even noticed?

Also, I do wonder if that check in do_generic_file_read() should just
unconditionally use MAX_LFS_FILESIZE, since the whole point there is
really about the index wrap-around, not about any underlying
filesystem limits per se.

And that's exactly what MAX_LFS_FILESIZE is - the maximum size that
fits in the page index.

              Linus

[toc] | [next] | [standalone]

#1718624

On 8/23/17 3:37 PM, Linus Torvalds wrote:
> On Wed, Aug 23, 2017 at 12:15 PM, Doug Nazar <nazard@nazar.ca> wrote:
>> The following commits cause short reads of block devices, however writes are
>> still allowed.
>>
>> c2a9737f45e2 ("vfs,mm: fix a dead loop in truncate_inode_pages_range()")
>> d05c5f7ba164 ("vfs,mm: fix return value of read() at s_maxbytes")
>>
>> When e2fsck sees this, it thinks it's a bad sector and tries to write a
>> block of nulls which overwrites the valid data.
> Hmm. Block devices shouldn't have issues with s_maxbytes, and I'm
> surprised that nobody has seen that before.
>
>> Device is LVM over 2 x RAID-5 on an old 32bit desktop.
>>
>> RO    RA   SSZ   BSZ   StartSec            Size   Device
>> rw  4096   512  4096          0   9748044840960 /dev/Storage/Main
> .. and the problem may be as simple as just a missing initialization
> of s_maxbytes for blockdev_superblock.
>
> Does the attcahed trivial one-liner fix things for you?
>
> Al, if it really is this simple, how come nobody even noticed?
>
> Also, I do wonder if that check in do_generic_file_read() should just
> unconditionally use MAX_LFS_FILESIZE, since the whole point there is
> really about the index wrap-around, not about any underlying
> filesystem limits per se.
>
> And that's exactly what MAX_LFS_FILESIZE is - the maximum size that
> fits in the page index.

It's compiling now, but I think it's already set to MAX_LFS_FILESIZE.

[  169.095127] ppos=80180006000, s_maxbytes=7ffffffffff, 
magic=0x62646576, type=bdev

Doug

[toc] | [prev] | [next] | [standalone]

#1718628

On Wed, Aug 23, 2017 at 12:53 PM, Doug Nazar <nazard@nazar.ca> wrote:
>
> It's compiling now, but I think it's already set to MAX_LFS_FILESIZE.
>
> [  169.095127] ppos=80180006000, s_maxbytes=7ffffffffff, magic=0x62646576,
> type=bdev

Oh, right you are - I'm much too used to 64-bit, where
MAX_LFS_FILESIZE is basically infinite, and was jusr assuming that it
was something like the UFS bug we had not that long ago that was due
to the 32-bit limit.

But yes, on 32-bit, we are limited by the 32-bit index into the page
cache, and we limit the index to 31 bits too, so we have (PAGE_SIZE <<
31) -1, which is that 7ffffffffff.

And that also explains why people haven't seen it. You do need

 (a) 32-bit environment

 (b) a disk larger than that 8TB in size

The *hard* limit for the page cache on a 32-bit environment should
actually be (PAGE_SIZE << 32)-PAGE_SIZE (that final PAGE_SIZE
subtraction is to make sure we don't generate that page cache with
index -1), so having a disk that is 16TB or larger is not going to
work, but your disk is right in that 8TB-16TB hole that used to work
and was broken by that check.

Anyway, that makes me feel better. I should have looked at your disk
size more, now I at least understand why nobody noticed before.

So just throw away my patch. That's wrong, and garbage.

The *right* patch is likely to just this instead:

  -#define MAX_LFS_FILESIZE       (((loff_t)PAGE_SIZE << (BITS_PER_LONG-1))-1)
  +#define MAX_LFS_FILESIZE       (((loff_t)PAGE_SIZE <<
BITS_PER_LONG)-PAGE_SIZE)

which should make MAX_LFS_FILESIZE be 0xffffffff000 and you disk size
should be ok.

                      Linus

[toc] | [prev] | [next] | [standalone]

#1718648

[Multipart message — attachments visible in raw view] — view raw

On Aug 23, 2017, at 2:13 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> On Wed, Aug 23, 2017 at 12:53 PM, Doug Nazar <nazard@nazar.ca> wrote:
>> 
>> It's compiling now, but I think it's already set to MAX_LFS_FILESIZE.
>> 
>> [  169.095127] ppos=80180006000, s_maxbytes=7ffffffffff, magic=0x62646576,
>> type=bdev
> 
> Oh, right you are - I'm much too used to 64-bit, where
> MAX_LFS_FILESIZE is basically infinite, and was jusr assuming that it
> was something like the UFS bug we had not that long ago that was due
> to the 32-bit limit.
> 
> But yes, on 32-bit, we are limited by the 32-bit index into the page
> cache, and we limit the index to 31 bits too, so we have (PAGE_SIZE <<
> 31) -1, which is that 7ffffffffff.
> 
> And that also explains why people haven't seen it. You do need
> 
> (a) 32-bit environment
> 
> (b) a disk larger than that 8TB in size
> 
> The *hard* limit for the page cache on a 32-bit environment should
> actually be (PAGE_SIZE << 32)-PAGE_SIZE (that final PAGE_SIZE
> subtraction is to make sure we don't generate that page cache with
> index -1), so having a disk that is 16TB or larger is not going to
> work, but your disk is right in that 8TB-16TB hole that used to work
> and was broken by that check.
> 
> Anyway, that makes me feel better. I should have looked at your disk
> size more, now I at least understand why nobody noticed before.
> 
> So just throw away my patch. That's wrong, and garbage.
> 
> The *right* patch is likely to just this instead:
> 
>  -#define MAX_LFS_FILESIZE       (((loff_t)PAGE_SIZE << (BITS_PER_LONG-1))-1)
>  +#define MAX_LFS_FILESIZE       (((loff_t)PAGE_SIZE <<
> BITS_PER_LONG)-PAGE_SIZE)
> 
> which should make MAX_LFS_FILESIZE be 0xffffffff000 and you disk size
> should be ok.

Doug,
I noticed while checking for other implications of changing MAX_LFS_FILESIZE
that fs/jfs/super.c is also working around this limit.  If you are going
to submit a patch for this, it also makes sense to fix jfs_fill_super() to
use MAX_LFS_FILESIZE instead of JFS rolling its own, something like:

	/* logical blocks are represented by 40 bits in pxd_t, etc.
	 * and page cache is indexed by long. */
	sb->s_maxbytes = min((u64)sb->s_blocksize) << 40,
                             MAX_LFS_FILESIZE);

It also looks like ocfs2_max_file_offset() is trying to avoid overflowing
the old 31-bit limit, and isn't using MAX_LFS_FILESIZE directly, so it will
now be wrong.  It looks like it could use "bitshift = 32; trim = bytes;",
but Joel or Mark should confirm.

Finally, there is a check in fs/super.c::mount_fs() that is verifying
s_maxbytes is not set too large, but this has been present since 2.6.32
and should probably be removed at this point, or changed to a BUG_ON()
(see commit 42cb56ae2ab for details).

Cheers, Andreas

[toc] | [prev] | [next] | [standalone]

#1719129

On 8/23/17 5:01 PM, Andreas Dilger wrote:
> Doug,
> I noticed while checking for other implications of changing MAX_LFS_FILESIZE
> that fs/jfs/super.c is also working around this limit.  If you are going
> to submit a patch for this, it also makes sense to fix jfs_fill_super() to
> use MAX_LFS_FILESIZE instead of JFS rolling its own, something like:
>
> 	/* logical blocks are represented by 40 bits in pxd_t, etc.
> 	 * and page cache is indexed by long. */
> 	sb->s_maxbytes = min((u64)sb->s_blocksize) << 40,
>                               MAX_LFS_FILESIZE);
>
> It also looks like ocfs2_max_file_offset() is trying to avoid overflowing
> the old 31-bit limit, and isn't using MAX_LFS_FILESIZE directly, so it will
> now be wrong.  It looks like it could use "bitshift = 32; trim = bytes;",
> but Joel or Mark should confirm.
>
> Finally, there is a check in fs/super.c::mount_fs() that is verifying
> s_maxbytes is not set too large, but this has been present since 2.6.32
> and should probably be removed at this point, or changed to a BUG_ON()
> (see commit 42cb56ae2ab for details).

I don't have any issue trying to write patches for those, but I have no 
domain knowledge
in the area or any way to test them.

 From a quick glance, jfs is locked to PSIZE (4096) so should be ok.
OCFS looks a little complex, and since it's a shared fs, little hesitant.

The check in fs/super.c, maybe that should be:

sb->s_maxbytes > MAX_LFS_FILESIZE

Actually, little confused, the comment says unsigned, but loff_t looks 
like its long long.
Maybe cast to u64 and check greater than?

Doug

[toc] | [prev] | [next] | [standalone]

#1719340

On 08/24/2017 05:20 AM, Doug Nazar wrote:
> On 8/23/17 5:01 PM, Andreas Dilger wrote:
>> Doug,
>> I noticed while checking for other implications of changing
>> MAX_LFS_FILESIZE
>> that fs/jfs/super.c is also working around this limit.  If you are going
>> to submit a patch for this, it also makes sense to fix
>> jfs_fill_super() to
>> use MAX_LFS_FILESIZE instead of JFS rolling its own, something like:
>>
>>     /* logical blocks are represented by 40 bits in pxd_t, etc.
>>      * and page cache is indexed by long. */
>>     sb->s_maxbytes = min((u64)sb->s_blocksize) << 40,
>>                               MAX_LFS_FILESIZE);
>>
>> It also looks like ocfs2_max_file_offset() is trying to avoid overflowing
>> the old 31-bit limit, and isn't using MAX_LFS_FILESIZE directly, so it
>> will
>> now be wrong.  It looks like it could use "bitshift = 32; trim = bytes;",
>> but Joel or Mark should confirm.
>>
>> Finally, there is a check in fs/super.c::mount_fs() that is verifying
>> s_maxbytes is not set too large, but this has been present since 2.6.32
>> and should probably be removed at this point, or changed to a BUG_ON()
>> (see commit 42cb56ae2ab for details).
> 
> I don't have any issue trying to write patches for those, but I have no
> domain knowledge
> in the area or any way to test them.

If you want to wrap the jfs change into this, I will be happy to test it
for you, or I could take care of jfs with a separate patch if you'd prefer.

> 
> From a quick glance, jfs is locked to PSIZE (4096) so should be ok.
> OCFS looks a little complex, and since it's a shared fs, little hesitant.
> 
> The check in fs/super.c, maybe that should be:
> 
> sb->s_maxbytes > MAX_LFS_FILESIZE
> 
> Actually, little confused, the comment says unsigned, but loff_t looks
> like its long long.
> Maybe cast to u64 and check greater than?
> 
> Doug
> 

[toc] | [prev] | [next] | [standalone]

#1719108

On 8/23/17 4:13 PM, Linus Torvalds wrote:

> Oh, right you are - I'm much too used to 64-bit, where
> MAX_LFS_FILESIZE is basically infinite, and was jusr assuming that it
> was something like the UFS bug we had not that long ago that was due
> to the 32-bit limit.
>
> But yes, on 32-bit, we are limited by the 32-bit index into the page
> cache, and we limit the index to 31 bits too, so we have (PAGE_SIZE <<
> 31) -1, which is that 7ffffffffff.

Yeah, it's an old organically grown storage server (install images, old 
vms, etc.) that
I can't convince myself it's worth upgrading.

> The *right* patch is likely to just this instead:
>
>    -#define MAX_LFS_FILESIZE       (((loff_t)PAGE_SIZE << (BITS_PER_LONG-1))-1)
>    +#define MAX_LFS_FILESIZE       (((loff_t)PAGE_SIZE <<
> BITS_PER_LONG)-PAGE_SIZE)
>
> which should make MAX_LFS_FILESIZE be 0xffffffff000 and you disk size
> should be ok.

That solves my issue. I'm curious if that check should also be in the 
write path. If the
check had been there too I wouldn't have ended up with any corruption.

I'm not sure if anything over 16TB will create/assemble (or if anybody 
else is crazy like me).

Doug

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web