From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:12875 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751111AbcFQFS2 (ORCPT ); Fri, 17 Jun 2016 01:18:28 -0400 Received: from pps.filterd (m0098399.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.11/8.16.0.11) with SMTP id u5H5Dpoc135344 for ; Fri, 17 Jun 2016 01:18:27 -0400 Received: from e28smtp06.in.ibm.com (e28smtp06.in.ibm.com [125.16.236.6]) by mx0a-001b2d01.pphosted.com with ESMTP id 23kq701w13-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Fri, 17 Jun 2016 01:18:27 -0400 Received: from localhost by e28smtp06.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 17 Jun 2016 10:48:24 +0530 Received: from d28relay02.in.ibm.com (d28relay02.in.ibm.com [9.184.220.59]) by d28dlp02.in.ibm.com (Postfix) with ESMTP id 6A1583940061 for ; Fri, 17 Jun 2016 10:48:18 +0530 (IST) Received: from d28av02.in.ibm.com (d28av02.in.ibm.com [9.184.220.64]) by d28relay02.in.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id u5H5IBo063504492 for ; Fri, 17 Jun 2016 10:48:11 +0530 Received: from d28av02.in.ibm.com (localhost [127.0.0.1]) by d28av02.in.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id u5H5IEdS010382 for ; Fri, 17 Jun 2016 10:48:15 +0530 From: Chandan Rajendra To: bo.li.liu@oracle.com Cc: linux-btrfs@vger.kernel.org, Eryu Guan , David Sterba Subject: Re: [PATCH] Btrfs: let super_stripesize match with sectorsize Date: Fri, 17 Jun 2016 10:48:05 +0530 In-Reply-To: <20160616170141.GB27257@localhost.localdomain> References: <1465940023-27773-1-git-send-email-bo.li.liu@oracle.com> <2409903.XvMUHL7gZ9@localhost.localdomain> <20160616170141.GB27257@localhost.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Message-Id: <69818794.Hm6NXvuvrY@reserved-192-168-1-6.rchland.ibm.com> Sender: linux-btrfs-owner@vger.kernel.org List-ID: On Thursday, June 16, 2016 10:01:41 AM Liu Bo wrote: > On Thu, Jun 16, 2016 at 01:53:59PM +0530, Chandan Rajendra wrote: > > On Wednesday, June 15, 2016 05:09:55 PM Liu Bo wrote: > > > On Wed, Jun 15, 2016 at 03:50:17PM +0530, Chandan Rajendra wrote: > > > > On Wednesday, June 15, 2016 09:12:28 AM Chandan Rajendra wrote: > > > > > Hello Liu Bo, > > > > > > > > > > We have to fix the following check in check_super() as well, > > > > > > > > > > if (btrfs_super_stripesize(sb) != 4096) { > > > > > > > > > > error("invalid stripesize %u", > > > > > btrfs_super_stripesize(sb)); > > > > > goto error_out; > > > > > > > > > > } > > > > > > > > > > i.e. btrfs_super_stripesize(sb) must be equal to > > > > > btrfs_super_sectorsize(sb). > > > > > > > > > > However in btrfs-progs (mkfs.c to be precise) since we had > > > > > stripesize > > > > > hardcoded to 4096, setting stripesize to the value of sectorsize in > > > > > mkfs.c will cause the following to occur when mkfs.btrfs is invoked > > > > > for > > > > > devices with existing Btrfs filesystem instances, > > > > > > > > > > NOTE: Assume we have changed the stripesize validation in > > > > > btrfs-progs' > > > > > check_super() to, > > > > > > > > > > if (btrfs_super_stripesize(sb) != > > > > > btrfs_super_sectorsize(sb)) { > > > > > > > > > > error("invalid stripesize %u", > > > > > btrfs_super_stripesize(sb)); > > > > > goto error_out; > > > > > > > > > > } > > > > > > > > > > main() > > > > > > > > > > for each device file passed as an argument, > > > > > > > > > > test_dev_for_mkfs() > > > > > > > > > > check_mounted > > > > > > > > > > check_mounted_where > > > > > > > > > > btrfs_scan_one_device > > > > > > > > > > btrfs_read_dev_super > > > > > > > > > > check_super() call will fail for existing filesystems > > > > > which > > > > > > > > > > have stripesize set to 4k. All existing filesystem instances will > > > > > fall > > > > > into > > > > > this category. > > > > > > > > > > This error value is pushed up the call stack and this causes the > > > > > device > > > > > to > > > > > not get added to the fs_devices_mnt list in check_mounted_where(). > > > > > Hence > > > > > we > > > > > would fail to correctly check the mount status of the multi-device > > > > > btrfs > > > > > filesystems. > > > > > > > > We can end up in the following scenario, > > > > - /dev/loop0, /dev/loop1 and /dev/loop2 are mounted as a single > > > > > > > > filesystem. The filesystem was created by an older version of > > > > mkfs.btrfs > > > > which set stripesize to 4k. > > > > > > > > - losetup -a > > > > > > > > /dev/loop0: [0030]:19477 (/root/disk-imgs/file-0.img) > > > > /dev/loop1: [0030]:16577 (/root/disk-imgs/file-1.img) > > > > /dev/loop2: [64770]:3423229 (/root/disk-imgs/file-2.img) > > > > > > > > - /etc/mtab lists only /dev/loop0 > > > > - "losetup /dev/loop4 /root/disk-imgs/file-1.img" > > > > > > > > The new mkfs.btrfs invoked as 'mkfs.btrfs -f /dev/loop4' succeeds > > > > even > > > > though /dev/loop1 has already been mounted and has > > > > /root/disk-imgs/file-1.img as its backing file. > > > > > > > > So IMHO the only solution is to have the stripesize check in > > > > check_super() > > > > to allow both '4k' and 'sectorsize' as valid values i.e. > > > > > > > > if ((btrfs_super_stripesize(sb) != 4096) > > > > > > > > && (btrfs_super_stripesize(sb) != btrfs_super_sectorsize(sb))) { > > > > > > > > error("invalid stripesize %u", > > > > btrfs_super_stripesize(sb)); > > > > goto error_out; > > > > > > > > } > > > > > > That's a good one. > > > > > > But if we go back to the original point, in the kernel side, > > > 1. in open_ctree(), root->stripesize = btrfs_super_stripesize(); > > > > > > 2. in find_free_extent(), > > > > > > ... > > > search_start = ALIGN(offset, root->stripesize); > > > ... > > > > > > 3. in btrfs_alloc_tree_block(), > > > > > > ... > > > ret = btrfs_reserve_extent(..., &ins, ...); > > > ... > > > buf = btrfs_init_new_buffer(trans, root, ins.objectid, level); > > > > > > 4. in btrfs_init_new_buffer(), > > > > > > ... > > > buf = btrfs_find_create_tree_block(root, bytenr); > > > ... > > > > > > Because 'num_bytes' we pass to find_free_extent() is aligned to > > > sectorsize, the free space we can find is aligned to sectorsize, > > > which means 'offset' in '1. find_free_extent()' is aligned to > > > sectorsize. > > > > > > If our stripesize is larger than sectorsize, say 4 * sectorsize, > > > for data allocation it's fine while for metadata block allocations it's > > > not. It is possible that when we allocate a new metadata block, we can > > > end up with an existing eb returned by '4. in btrfs_init_new_buffer()'. > > > > Liu, I am sorry ... I am unable to visualize a scenario where the above > > described scenario could happen. Can you please provide an example? > > Sure, imagine that sectorsize is 4k and stripesize is 16k, > and a tree root's eb has eb->start = 12599296 (12582912 + 16384, a typical > bytenr in btrfs) which is aligned to 4k, and when CoW happens on another > eb, > > __btrfs_cow_block() > ->btrfs_alloc_tree_block() > ->btrfs_reserve_extent() > ->find_free_extent() > ->btrfs_init_new_buffer() > > btrfs_reserve_extent() can return 12599296 for the new eb even if what > it've found is (12582912 + 4096), but > after 'search_start = ALIGN(offset, root->stripesize)', it gets to > 12599296. > > In btrfs_init_new_buffer, we search eb tree by bytenr=12599296 and > get tree root's eb, the following btrfs_tree_lock will scream. > > The example is taken from > btrfs-progs/tests/fuzz-tests/images/superblock-stripsize-bogus.raw.xz > ah, this is indeed possible when nodesize is same as sectorsize i.e. 4k. Thanks for the explaination Liubo. The new validation patches that I have posted (http://news.gmane.org/find-root.php?message_id=1466095078%2d25726%2d1%2dgit%2dsend%2demail%2dchandan%40linux.vnet.ibm.com and http://news.gmane.org/find-root.php?message_id=1466095109%2d26044%2d1%2dgit%2dsend%2demail%2dchandan%40linux.vnet.ibm.com) restrict the stripesize to be either sectorsize or 4096. So I think these restrictions are good enough to make sure we don't get into the situation explained by you. -- chandan