Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758254AbcKCPkh (ORCPT ); Thu, 3 Nov 2016 11:40:37 -0400 Received: from mail-vk0-f68.google.com ([209.85.213.68]:35382 "EHLO mail-vk0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758217AbcKCPkf (ORCPT ); Thu, 3 Nov 2016 11:40:35 -0400 MIME-Version: 1.0 In-Reply-To: <20161103142644.GA15480@lst.de> References: <1478181846-88863-1-git-send-email-hare@suse.de> <1478181846-88863-3-git-send-email-hare@suse.de> <20161103142644.GA15480@lst.de> From: Ming Lei Date: Thu, 3 Nov 2016 23:40:34 +0800 Message-ID: Subject: Re: [PATCH 2/2] loop: support 4k physical blocksize To: Christoph Hellwig Cc: Hannes Reinecke , Jens Axboe , Alexander Graf , Martin Schwidefsky , Linux Kernel Mailing List , Hannes Reinecke Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2851 Lines: 65 On Thu, Nov 3, 2016 at 10:26 PM, Christoph Hellwig wrote: >> -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. This patch uses backing fs block size to emulate physical block size of loop block device, and the logical block size of loop is set from user space, seems the idea is correct. > >> + 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. -- Ming Lei