Re: [PATCH 1/4] loop: Enable correct physical blocksize

[Hannes Reinecke]

On 11/13/2015 09:57 PM, Jeff Moyer wrote:

Hi Hannes,

Hannes Reinecke  writes:


When running on files the physical blocksize is actually 4k,


How did you come to that conclusion?  Are you basing it on the file
system block size?  If so, that's configurable at mkfs time and can be
anything from 512 bytes to 64k on current in-tree file systems that I
know of (depending on platform, of course).


loop.c does this (in do_loop_switch()):

mapping_set_gfp_mask(old_file->f_mapping, lo->old_gfp_mask);
lo->lo_backing_file = file;
lo->lo_blocksize = S_ISBLK(mapping->host->i_mode) ?
mapping->host->i_bdev->bd_block_size : PAGE_SIZE;
lo->old_gfp_mask = mapping_gfp_mask(mapping);

So either it's a block device, then we're taking the blocksize of 
the underlying device, or we're using PAGE_SIZE.

Which is architecture dependent, of course.


The main use for physical block size, as I understand it, is to allow
partitioning utilities to place partitions on physical block boundaries
of the underlying storage.  The benefit of that is to avoid
read-modify-writes for I/O which is naturally sized and aligned.  If we
carry that forward to loop, then I think it does makes sense to key off
of the file system block size, but the fact remains that 4k is not
universal.

The main point here is that some utilities (eg bootloaders) need to 
know the _physical_ location of a particular blob, for which it 
needs to know the physical blocksize.



So, I think the idea is sound, but you should be setting the physical
block size to sb->s_blocksize.  And I don't see any reason why we
wouldn't do this by default, do you?


Neither do I. But the code doesn't treat it that way, so I elected
to stay with the current version.


If you end up reposting this patch, would you mind including more of
this rationale in your commit message?


Sure.

Cheers,

Hannes
--
Dr. Hannes ReineckezSeries & Storage
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] loop: Enable correct physical blocksize

[Hannes Reinecke]

On 11/13/2015 09:57 PM, Jeff Moyer wrote:

Hi Hannes,

Hannes Reinecke  writes:


When running on files the physical blocksize is actually 4k,


How did you come to that conclusion?  Are you basing it on the file
system block size?  If so, that's configurable at mkfs time and can be
anything from 512 bytes to 64k on current in-tree file systems that I
know of (depending on platform, of course).


loop.c does this (in do_loop_switch()):

mapping_set_gfp_mask(old_file->f_mapping, lo->old_gfp_mask);
lo->lo_backing_file = file;
lo->lo_blocksize = S_ISBLK(mapping->host->i_mode) ?
mapping->host->i_bdev->bd_block_size : PAGE_SIZE;
lo->old_gfp_mask = mapping_gfp_mask(mapping);

So either it's a block device, then we're taking the blocksize of 
the underlying device, or we're using PAGE_SIZE.

Which is architecture dependent, of course.


The main use for physical block size, as I understand it, is to allow
partitioning utilities to place partitions on physical block boundaries
of the underlying storage.  The benefit of that is to avoid
read-modify-writes for I/O which is naturally sized and aligned.  If we
carry that forward to loop, then I think it does makes sense to key off
of the file system block size, but the fact remains that 4k is not
universal.

The main point here is that some utilities (eg bootloaders) need to 
know the _physical_ location of a particular blob, for which it 
needs to know the physical blocksize.



So, I think the idea is sound, but you should be setting the physical
block size to sb->s_blocksize.  And I don't see any reason why we
wouldn't do this by default, do you?


Neither do I. But the code doesn't treat it that way, so I elected
to stay with the current version.


If you end up reposting this patch, would you mind including more of
this rationale in your commit message?


Sure.

Cheers,

Hannes
--
Dr. Hannes ReineckezSeries & Storage
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] loop: Enable correct physical blocksize

[Ming Lei]

On Tue, Nov 10, 2015 at 4:13 PM, Hannes Reinecke  wrote:
> When running on files the physical blocksize is actually 4k,
> so we should be announcing it as such. This is enabled with
> a new LO_FLAGS_BLOCKSIZE flag value to the existing
> loop_set_status ioctl.

LO_FLAGS_BLOCKSIZE is defined in patch 3/4, and you use
it too early in patch 1/4.

>
> Signed-off-by: Hannes Reinecke 
> ---
>  drivers/block/loop.c | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 423f4ca..e790487 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -233,6 +233,8 @@ figure_loop_size(struct loop_device *lo, loff_t offset, 
> loff_t sizelimit)
> lo->lo_offset = offset;
> if (lo->lo_sizelimit != sizelimit)
> lo->lo_sizelimit = sizelimit;
> +   if (lo->lo_flags & LO_FLAGS_BLOCKSIZE)
> +   blk_queue_physical_block_size(lo->lo_queue, lo->lo_blocksize);
> set_capacity(lo->lo_disk, x);
> bd_set_size(bdev, (loff_t)get_capacity(bdev->bd_disk) << 9);
> /* let user-space know about the new size */
> @@ -1092,6 +1094,7 @@ loop_set_status(struct loop_device *lo, const struct 
> loop_info64 *info)
> int err;
> struct loop_func_table *xfer;
> kuid_t uid = current_uid();
> +   int lo_flags = lo->lo_flags;
>
> if (lo->lo_encrypt_key_size &&
> !uid_eq(lo->lo_key_owner, uid) &&
> @@ -1121,8 +1124,12 @@ loop_set_status(struct loop_device *lo, const struct 
> loop_info64 *info)
> if (err)
> return err;
>
> +   if (info->lo_flags & LO_FLAGS_BLOCKSIZE)
> +   lo->lo_flags |= LO_FLAGS_BLOCKSIZE;
> +
> if (lo->lo_offset != info->lo_offset ||
> -   lo->lo_sizelimit != info->lo_sizelimit)
> +   lo->lo_sizelimit != info->lo_sizelimit ||
> +   lo->lo_flags != lo_flags)
> if (figure_loop_size(lo, info->lo_offset, info->lo_sizelimit))
> return -EFBIG;
>
> --
> 1.8.5.6
>



-- 
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] loop: Enable correct physical blocksize

[Ming Lei]

On Tue, Nov 10, 2015 at 4:13 PM, Hannes Reinecke  wrote:
> When running on files the physical blocksize is actually 4k,
> so we should be announcing it as such. This is enabled with
> a new LO_FLAGS_BLOCKSIZE flag value to the existing
> loop_set_status ioctl.

LO_FLAGS_BLOCKSIZE is defined in patch 3/4, and you use
it too early in patch 1/4.

>
> Signed-off-by: Hannes Reinecke 
> ---
>  drivers/block/loop.c | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 423f4ca..e790487 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -233,6 +233,8 @@ figure_loop_size(struct loop_device *lo, loff_t offset, 
> loff_t sizelimit)
> lo->lo_offset = offset;
> if (lo->lo_sizelimit != sizelimit)
> lo->lo_sizelimit = sizelimit;
> +   if (lo->lo_flags & LO_FLAGS_BLOCKSIZE)
> +   blk_queue_physical_block_size(lo->lo_queue, lo->lo_blocksize);
> set_capacity(lo->lo_disk, x);
> bd_set_size(bdev, (loff_t)get_capacity(bdev->bd_disk) << 9);
> /* let user-space know about the new size */
> @@ -1092,6 +1094,7 @@ loop_set_status(struct loop_device *lo, const struct 
> loop_info64 *info)
> int err;
> struct loop_func_table *xfer;
> kuid_t uid = current_uid();
> +   int lo_flags = lo->lo_flags;
>
> if (lo->lo_encrypt_key_size &&
> !uid_eq(lo->lo_key_owner, uid) &&
> @@ -1121,8 +1124,12 @@ loop_set_status(struct loop_device *lo, const struct 
> loop_info64 *info)
> if (err)
> return err;
>
> +   if (info->lo_flags & LO_FLAGS_BLOCKSIZE)
> +   lo->lo_flags |= LO_FLAGS_BLOCKSIZE;
> +
> if (lo->lo_offset != info->lo_offset ||
> -   lo->lo_sizelimit != info->lo_sizelimit)
> +   lo->lo_sizelimit != info->lo_sizelimit ||
> +   lo->lo_flags != lo_flags)
> if (figure_loop_size(lo, info->lo_offset, info->lo_sizelimit))
> return -EFBIG;
>
> --
> 1.8.5.6
>



-- 
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] loop: Enable correct physical blocksize

[Jeff Moyer]

Hi Hannes,

Hannes Reinecke  writes:

> When running on files the physical blocksize is actually 4k,

How did you come to that conclusion?  Are you basing it on the file
system block size?  If so, that's configurable at mkfs time and can be
anything from 512 bytes to 64k on current in-tree file systems that I
know of (depending on platform, of course).

The main use for physical block size, as I understand it, is to allow
partitioning utilities to place partitions on physical block boundaries
of the underlying storage.  The benefit of that is to avoid
read-modify-writes for I/O which is naturally sized and aligned.  If we
carry that forward to loop, then I think it does makes sense to key off
of the file system block size, but the fact remains that 4k is not
universal.

So, I think the idea is sound, but you should be setting the physical
block size to sb->s_blocksize.  And I don't see any reason why we
wouldn't do this by default, do you?

If you end up reposting this patch, would you mind including more of
this rationale in your commit message?

Thanks!
Jeff
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] loop: Enable correct physical blocksize

[Jeff Moyer]

Hi Hannes,

Hannes Reinecke  writes:

> When running on files the physical blocksize is actually 4k,

How did you come to that conclusion?  Are you basing it on the file
system block size?  If so, that's configurable at mkfs time and can be
anything from 512 bytes to 64k on current in-tree file systems that I
know of (depending on platform, of course).

The main use for physical block size, as I understand it, is to allow
partitioning utilities to place partitions on physical block boundaries
of the underlying storage.  The benefit of that is to avoid
read-modify-writes for I/O which is naturally sized and aligned.  If we
carry that forward to loop, then I think it does makes sense to key off
of the file system block size, but the fact remains that 4k is not
universal.

So, I think the idea is sound, but you should be setting the physical
block size to sb->s_blocksize.  And I don't see any reason why we
wouldn't do this by default, do you?

If you end up reposting this patch, would you mind including more of
this rationale in your commit message?

Thanks!
Jeff
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] loop: Enable correct physical blocksize

[Christoph Hellwig]

On Mon, Jul 27, 2015 at 07:59:47AM +0200, Hannes Reinecke wrote:
> On 07/27/2015 07:15 AM, Christoph Hellwig wrote:
> > On Fri, Jul 17, 2015 at 09:27:04AM +0200, Hannes Reinecke wrote:
> >> When running on files the physical blocksize is actually 4k,
> >> so we should be announcing it as such. This is enabled with
> >> a new LO_FLAGS_BLOCKSIZE flag value to the existing ioctl.
> > 
> > The flag is only used in this patch, but not actually defined anywhere.
> > 
> Ah, Merge error.
> I'll fix it up.

Can you resen the series?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] loop: Enable correct physical blocksize

[Christoph Hellwig]

On Mon, Jul 27, 2015 at 07:59:47AM +0200, Hannes Reinecke wrote:
 On 07/27/2015 07:15 AM, Christoph Hellwig wrote:
  On Fri, Jul 17, 2015 at 09:27:04AM +0200, Hannes Reinecke wrote:
  When running on files the physical blocksize is actually 4k,
  so we should be announcing it as such. This is enabled with
  a new LO_FLAGS_BLOCKSIZE flag value to the existing ioctl.
  
  The flag is only used in this patch, but not actually defined anywhere.
  
 Ah, Merge error.
 I'll fix it up.

Can you resen the series?
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] loop: Enable correct physical blocksize

[Hannes Reinecke]

On 07/27/2015 07:15 AM, Christoph Hellwig wrote:
> On Fri, Jul 17, 2015 at 09:27:04AM +0200, Hannes Reinecke wrote:
>> When running on files the physical blocksize is actually 4k,
>> so we should be announcing it as such. This is enabled with
>> a new LO_FLAGS_BLOCKSIZE flag value to the existing ioctl.
> 
> The flag is only used in this patch, but not actually defined anywhere.
> 
Ah, Merge error.
I'll fix it up.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] loop: Enable correct physical blocksize

[Hannes Reinecke]

On 07/27/2015 07:15 AM, Christoph Hellwig wrote:
 On Fri, Jul 17, 2015 at 09:27:04AM +0200, Hannes Reinecke wrote:
 When running on files the physical blocksize is actually 4k,
 so we should be announcing it as such. This is enabled with
 a new LO_FLAGS_BLOCKSIZE flag value to the existing ioctl.
 
 The flag is only used in this patch, but not actually defined anywhere.
 
Ah, Merge error.
I'll fix it up.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke   zSeries  Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] loop: Enable correct physical blocksize

[Christoph Hellwig]

On Fri, Jul 17, 2015 at 09:27:04AM +0200, Hannes Reinecke wrote:
> When running on files the physical blocksize is actually 4k,
> so we should be announcing it as such. This is enabled with
> a new LO_FLAGS_BLOCKSIZE flag value to the existing ioctl.

The flag is only used in this patch, but not actually defined anywhere.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] loop: Enable correct physical blocksize

[Christoph Hellwig]

On Fri, Jul 17, 2015 at 09:27:04AM +0200, Hannes Reinecke wrote:
 When running on files the physical blocksize is actually 4k,
 so we should be announcing it as such. This is enabled with
 a new LO_FLAGS_BLOCKSIZE flag value to the existing ioctl.

The flag is only used in this patch, but not actually defined anywhere.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/