Prev: Tight check of pfn_valid on sparsemem
Next: Fondation De France Notification!!!!
From: Andreas Dilger on 12 Jul 2010 20:30
On 2010-07-11, at 11:04, Patrick J. LoPresti wrote:
> +/* Check to make sure entire volume is addressable on this system.
> + Requires osb_clusters_at_boot to be valid and for the journal to
> + have been initialized by ocfs2_journal_init(). */
> +static int ocfs2_check_addressable(struct ocfs2_super *osb)
> +{
> + /* Absolute addressability check (borrowed from ext4/super.c) */
> + if ((max_block >
> + (sector_t)(~0LL) >> (osb->sb->s_blocksize_bits - 9)) ||
> + (max_block > (pgoff_t)(~0LL) >> (PAGE_CACHE_SHIFT -
> + osb->sb->s_blocksize_bits))) {
> + mlog(ML_ERROR, "Volume too large "
> + "to mount safely on this system");
> + status = -EFBIG;
> + goto out;
> + }

This hunk of code is actually in several filesystems. It wouldn't be a bad idea to make it a library function that can be called by the filesystem to check the kernel page cache and block layer can handle these large filesystems.

Cheers, Andreas





--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Patrick J. LoPresti on 12 Jul 2010 21:10
On Mon, Jul 12, 2010 at 5:21 PM, Andreas Dilger <adilger(a)dilger.ca> wrote:
> On 2010-07-11, at 11:04, Patrick J. LoPresti wrote:
> >
>> + � � /* Absolute addressability check (borrowed from ext4/super.c) */
>> + � � if ((max_block >
>> + � � � � �(sector_t)(~0LL) >> (osb->sb->s_blocksize_bits - 9)) ||
>> + � � � � (max_block > (pgoff_t)(~0LL) >> (PAGE_CACHE_SHIFT -
>> + � � � � � � � � � � � � � � � � � � � � �osb->sb->s_blocksize_bits))) {
>> + � � � � � � mlog(ML_ERROR, "Volume too large "
>> + � � � � � � � � �"to mount safely on this system");
>> + � � � � � � status = -EFBIG;
>> + � � � � � � goto out;
>> + � � }
>
> This hunk of code is actually in several filesystems. �It wouldn't be a bad idea to make it a library function that can be called by the filesystem to check the kernel page cache and block layer can handle these large filesystems.

True, but some of them do it differently (e.g. see the #if switch in
xfs_sb_validate_fsb_count). Tracking down all variants and changing
them is a much larger task than my simple patch.

Are you suggesting I need to do this before my patch is accepted at
all? Or is this a refactoring that can happen later?

- Pat
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Dave Chinner on 12 Jul 2010 21:30
On Mon, Jul 12, 2010 at 06:08:51PM -0700, Patrick J. LoPresti wrote:
> On Mon, Jul 12, 2010 at 5:21 PM, Andreas Dilger <adilger(a)dilger.ca> wrote:
> > On 2010-07-11, at 11:04, Patrick J. LoPresti wrote:
> > >
> >> + � � /* Absolute addressability check (borrowed from ext4/super.c) */
> >> + � � if ((max_block >
> >> + � � � � �(sector_t)(~0LL) >> (osb->sb->s_blocksize_bits - 9)) ||
> >> + � � � � (max_block > (pgoff_t)(~0LL) >> (PAGE_CACHE_SHIFT -
> >> + � � � � � � � � � � � � � � � � � � � � �osb->sb->s_blocksize_bits))) {
> >> + � � � � � � mlog(ML_ERROR, "Volume too large "
> >> + � � � � � � � � �"to mount safely on this system");
> >> + � � � � � � status = -EFBIG;
> >> + � � � � � � goto out;
> >> + � � }
> >
> > This hunk of code is actually in several filesystems. �It wouldn't be a bad idea to make it a library function that can be called by the filesystem to check the kernel page cache and block layer can handle these large filesystems.
>
> True, but some of them do it differently (e.g. see the #if switch in
> xfs_sb_validate_fsb_count). Tracking down all variants and changing
> them is a much larger task than my simple patch.

The XFS code is different to the above because there is still a 16TB
size limit on 32 bit systemsi (i.e. page cache address limits). IOWs,
you can't just remove the above 16TB check unless you (i.e. OCFS2)
handle >16TB block devices on 32 bit systems correctly...

Cheers,

Dave.
--
Dave Chinner
david(a)fromorbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Patrick J. LoPresti on 12 Jul 2010 21:40
On Mon, Jul 12, 2010 at 6:25 PM, Dave Chinner <david(a)fromorbit.com> wrote:
>
> The XFS code is different to the above because there is still a 16TB
> size limit on 32 bit systemsi (i.e. page cache address limits). IOWs,
> you can't just remove the above 16TB check unless you (i.e. OCFS2)
> handle >16TB block devices on 32 bit systems correctly...

If you look at my patch, you will see that is precisely what it does.
As the comments indicate, it uses the exact same check as ext4, which
will correctly refuse to mount huge volumes on 32-bit systems.

The XFS test appears to be the same thing written a little
differently. Andreas is suggesting that somebody should factor out
this check into a common library routine. That sounds like a fine
idea, but it also sounds orthogonal to the (simple and useful) patch I
am attempting to submit.

- Pat
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Andreas Dilger on 13 Jul 2010 00:50
On 2010-07-12, at 19:08, Patrick J. LoPresti wrote:
> On Mon, Jul 12, 2010 at 5:21 PM, Andreas Dilger <adilger(a)dilger.ca> wrote:
>> On 2010-07-11, at 11:04, Patrick J. LoPresti wrote:
>>>
>>> + /* Absolute addressability check (borrowed from ext4/super.c) */
>>> + if ((max_block >
>>> + (sector_t)(~0LL) >> (osb->sb->s_blocksize_bits - 9)) ||
>>> + (max_block > (pgoff_t)(~0LL) >> (PAGE_CACHE_SHIFT -
>>> + osb->sb->s_blocksize_bits))) {
>>> + mlog(ML_ERROR, "Volume too large "
>>> + "to mount safely on this system");
>>> + status = -EFBIG;
>>> + goto out;
>>> + }
>>
>> This hunk of code is actually in several filesystems. It wouldn't be a bad idea to make it a library function that can be called by the filesystem to check the kernel page cache and block layer can handle these large filesystems.
>
> True, but some of them do it differently (e.g. see the #if switch in
> xfs_sb_validate_fsb_count). Tracking down all variants and changing
> them is a much larger task than my simple patch.
>
> Are you suggesting I need to do this before my patch is accepted at
> all? Or is this a refactoring that can happen later?

I'm just suggesting it should be done at some point. I thought it would be better to do it first, rather than add yet another copy of this code. That said, I hate to block useful fixes because of cleanup (and I have no control over OCFS2 anyway :-). However, I've found that once the fix is in people usually forget (or become too busy) to do the cleanup and it just lingers on unseen.

Cheers, Andreas





--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2
Prev: Tight check of pfn_valid on sparsemem
Next: Fondation De France Notification!!!!