Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752270AbbKMVmi (ORCPT ); Fri, 13 Nov 2015 16:42:38 -0500 Received: from mx1.redhat.com ([209.132.183.28]:43525 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752161AbbKMVmD (ORCPT ); Fri, 13 Nov 2015 16:42:03 -0500 From: Jeff Moyer To: Hannes Reinecke Cc: Jens Axboe , Alexander Graf , Christoph Hellwig , Ming Lei , linux-kernel@vger.kernel.org Subject: Re: [PATCH 4/4] loop: Pass logical blocksize in 'lo_init[0]' ioctl field References: <1447143190-44589-1-git-send-email-hare@suse.de> <1447143190-44589-5-git-send-email-hare@suse.de> X-PGP-KeyID: 1F78E1B4 X-PGP-CertKey: F6FE 280D 8293 F72C 65FD 5A58 1FF8 A7CA 1F78 E1B4 X-PCLoadLetter: What the f**k does that mean? Date: Fri, 13 Nov 2015 16:42:01 -0500 In-Reply-To: <1447143190-44589-5-git-send-email-hare@suse.de> (Hannes Reinecke's message of "Tue, 10 Nov 2015 09:13:10 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3916 Lines: 98 Hi Hannes, Hannes Reinecke writes: > The current LOOP_SET_STATUS64 ioctl has two unused fields > 'init[2]', which can be used in conjunction with the > LO_FLAGS_BLOCKSIZE flag to pass in the new logical blocksize. I don't see a reason to set LO_FLAGS_BLOCKSIZE inside of the loop_device->lo_flags. It's not a flag that gets toggled; rather, it's a flag that indicates that we're attempting to change the block size. The block size is the persistent state. To further clarify, I'm okay with passing it in info->lo_flags, I just don't like that it gets also set in the struct loop_device. I also think that you should spell out logical_block_size, instead of having physical_block_size and block_size. It's just easier to read, in my opinion. If you take my suggestion to set the physical block size automatically, I think that will clean things up some, too. I'm looking forward to v2. And just FYI, nothing looked horrible in patch 3/4, but I'm not going to ack it until I see the v2 posting, since I think it will change at least a little. Cheers, Jeff > Signed-off-by: Hannes Reinecke > --- > drivers/block/loop.c | 25 ++++++++++++++++++++----- > 1 file changed, 20 insertions(+), 5 deletions(-) > > diff --git a/drivers/block/loop.c b/drivers/block/loop.c > index d0b8754..6723e5e 100644 > --- a/drivers/block/loop.c > +++ b/drivers/block/loop.c > @@ -221,7 +221,8 @@ static void __loop_update_dio(struct loop_device *lo, bool dio) > } > > static int > -figure_loop_size(struct loop_device *lo, loff_t offset, loff_t sizelimit) > +figure_loop_size(struct loop_device *lo, loff_t offset, loff_t sizelimit, > + loff_t logical_blocksize) > { > loff_t size = get_size(offset, sizelimit, lo->lo_backing_file); > sector_t x = (sector_t)size; > @@ -234,6 +235,8 @@ figure_loop_size(struct loop_device *lo, loff_t offset, loff_t sizelimit) > if (lo->lo_sizelimit != sizelimit) > lo->lo_sizelimit = sizelimit; > if (lo->lo_flags & LO_FLAGS_BLOCKSIZE) { > + if (lo->lo_logical_blocksize != logical_blocksize) > + lo->lo_logical_blocksize = logical_blocksize; > blk_queue_physical_block_size(lo->lo_queue, lo->lo_blocksize); > blk_queue_logical_block_size(lo->lo_queue, > lo->lo_logical_blocksize); > @@ -1129,13 +1132,24 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info) > if (err) > return err; > > - if (info->lo_flags & LO_FLAGS_BLOCKSIZE) > + if (info->lo_flags & LO_FLAGS_BLOCKSIZE) { > lo->lo_flags |= LO_FLAGS_BLOCKSIZE; > + if ((info->lo_init[0] != 512) && > + (info->lo_init[0] != 1024) && > + (info->lo_init[0] != 2048) && > + (info->lo_init[0] != 4096)) > + return -EINVAL; > + if (info->lo_init[0] > lo->lo_blocksize) > + return -EINVAL; > + } > > if (lo->lo_offset != info->lo_offset || > lo->lo_sizelimit != info->lo_sizelimit || > - lo->lo_flags != lo_flags) > - if (figure_loop_size(lo, info->lo_offset, info->lo_sizelimit)) > + lo->lo_flags != lo_flags || > + ((lo->lo_flags & LO_FLAGS_BLOCKSIZE) && > + (lo->lo_logical_blocksize != info->lo_init[0]))) > + if (figure_loop_size(lo, info->lo_offset, info->lo_sizelimit, > + info->lo_init[0])) > return -EFBIG; > > loop_config_discard(lo); > @@ -1320,7 +1334,8 @@ static int loop_set_capacity(struct loop_device *lo) > if (unlikely(lo->lo_state != Lo_bound)) > return -ENXIO; > > - return figure_loop_size(lo, lo->lo_offset, lo->lo_sizelimit); > + return figure_loop_size(lo, lo->lo_offset, lo->lo_sizelimit, > + lo->lo_logical_blocksize); > } > > static int loop_set_dio(struct loop_device *lo, unsigned long arg) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/