Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757791AbcKCO0t (ORCPT ); Thu, 3 Nov 2016 10:26:49 -0400 Received: from verein.lst.de ([213.95.11.211]:41601 "EHLO newverein.lst.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757501AbcKCO0s (ORCPT ); Thu, 3 Nov 2016 10:26:48 -0400 Date: Thu, 3 Nov 2016 15:26:44 +0100 From: Christoph Hellwig To: Hannes Reinecke Cc: Jens Axboe , Christoph Hellwig , Alexander Graf , Ming Lei , Martin Schwidefsky , linux-kernel@vger.kernel.org, Hannes Reinecke Subject: Re: [PATCH 2/2] loop: support 4k physical blocksize Message-ID: <20161103142644.GA15480@lst.de> References: <1478181846-88863-1-git-send-email-hare@suse.de> <1478181846-88863-3-git-send-email-hare@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1478181846-88863-3-git-send-email-hare@suse.de> User-Agent: Mutt/1.5.17 (2007-11-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2219 Lines: 54 > -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; > @@ -233,6 +234,12 @@ 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) { > + 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); > + } I've looked how the existing code uses lo_blocksize and this whole thing confuses me to no end. Can we have all the blocksize assignments (i.e. including the existing lo_blocksize assignments) in one single place and documented? Especialy as it seems so far lo_blocksize seems to be the "fs" blocksize as set by set_blocksize, which seems totally wrong to be set by loop at all. > + 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; No need for the inner braces. Also please find a way to have a descriptive name for the field, be that an anonymous union, or a #define. > + (lo->lo_logical_blocksize != info->lo_init[0]))) Same comment about the inner braces here. > + if (figure_loop_size(lo, info->lo_offset, info->lo_sizelimit, > + info->lo_init[0])) > return -EFBIG; > > loop_config_discard(lo); > @@ -1303,7 +1328,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); I'd say drop all the arguments but lo here (maybe in a prep patch) as passing them all seems pointless and confusing.