From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from cn.fujitsu.com ([59.151.112.132]:9543 "EHLO heian.cn.fujitsu.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S932145AbaFJBtx convert rfc822-to-8bit (ORCPT ); Mon, 9 Jun 2014 21:49:53 -0400 Message-ID: <5396647E.6000104@cn.fujitsu.com> Date: Tue, 10 Jun 2014 09:50:54 +0800 From: Qu Wenruo MIME-Version: 1.0 To: Anand Jain , CC: , , , Subject: Re: [PATCH V3] Btrfs: device_list_add() should not update list when mounted References: <1402025201-17140-1-git-send-email-anand.jain@oracle.com> <53965E8C.6050106@cn.fujitsu.com> <539663E1.8050405@oracle.com> In-Reply-To: <539663E1.8050405@oracle.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Sender: linux-btrfs-owner@vger.kernel.org List-ID: -------- Original Message -------- Subject: Re: [PATCH V3] Btrfs: device_list_add() should not update list when mounted From: Anand Jain To: Qu Wenruo , linux-btrfs@vger.kernel.org Date: 2014年06月10日 09:48 > > > On 10/06/14 09:25, Qu Wenruo wrote: >> >> -------- Original Message -------- >> Subject: [PATCH V3] Btrfs: device_list_add() should not update list when >> mounted >> From: Anand Jain >> To: linux-btrfs@vger.kernel.org >> Date: 2014年06月06日 11:26 >>> (looks like there was some sendmail problem I don't see this in the >>> btrfs list, >>> sending again. sorry for multiple copies if any). >>> >>> device_list_add() is called when user runs btrfs dev scan, which would >>> add >>> any btrfs device into the btrfs_fs_devices list. >>> >>> Now think of a mounted btrfs. And a new device which contains the a SB >>> from the mounted btrfs devices. >>> >>> In this situation when user runs btrfs dev scan, the current code would >>> just replace existing device with the new device. >>> >>> Which is to note that old device is neither closed nor gracefully >>> removed from the btrfs. >>> >>> The FS is still operational with the old bdev however the device name >>> is the btrfs_device is new which is provided by the btrfs dev scan. >>> >>> reproducer: >>> >>> devmgt[1] detach /dev/sdc >>> >>> replace the missing disk /dev/sdc >>> >>> btrfs rep start -f 1 /dev/sde /btrfs >>> Label: none uuid: 5dc0aaf4-4683-4050-b2d6-5ebe5f5cd120 >>> Total devices 2 FS bytes used 32.00KiB >>> devid 1 size 958.94MiB used 115.88MiB path /dev/sde >>> devid 2 size 958.94MiB used 103.88MiB path /dev/sdd >>> >>> make /dev/sdc to reappear >>> >>> devmgt attach host2 >>> >>> btrfs dev scan >>> >>> btrfs fi show -m >>> Label: none uuid: 5dc0aaf4-4683-4050-b2d6-5ebe5f5cd120^M >>> Total devices 2 FS bytes used 32.00KiB^M >>> devid 1 size 958.94MiB used 115.88MiB path /dev/sdc <- >>> Wrong. >>> devid 2 size 958.94MiB used 103.88MiB path /dev/sdd >>> >>> since /dev/sdc has been replaced with /dev/sde, the /dev/sdc >>> shouldn't be >>> part of the btrfs-fsid when it reappears. If user want it to be part >>> of it >>> then sys admin should be using btrfs device add instead. >>> >>> [1] github.com/anajain/devmgt.git >>> >>> Signed-off-by: Anand Jain >>> --- >>> v2->v3: simpler commit and comment message >>> v1->v2: remove '---' in commit messages which conflict with git am >>> >>> fs/btrfs/volumes.c | 33 ++++++++++++++++++++++++++++++++- >>> 1 file changed, 32 insertions(+), 1 deletion(-) >>> >>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >>> index bb2cd66..605d9ef 100644 >>> --- a/fs/btrfs/volumes.c >>> +++ b/fs/btrfs/volumes.c >>> @@ -472,9 +472,12 @@ static noinline int device_list_add(const char >>> *path, >>> device = __find_device(&fs_devices->devices, devid, >>> disk_super->dev_item.uuid); >>> } >>> + >>> if (!device) { >>> - if (fs_devices->opened) >>> + if (fs_devices->opened) { >>> + printk(KERN_INFO "BTRFS: device list add failed, FS is >>> open"); >>> return -EBUSY; >>> + } >>> device = btrfs_alloc_device(NULL, &devid, >>> disk_super->dev_item.uuid); >>> @@ -497,6 +500,34 @@ static noinline int device_list_add(const char >>> *path, >>> device->fs_devices = fs_devices; >>> } else if (!device->name || strcmp(device->name->str, path)) { >>> + /* >>> + * When FS is already mounted. >>> + * 1. If you are here and if the device->name is NULL that >>> means >>> + * this device was missing at time of FS mount. >>> + * 2. If you are here and if the device->name is different >>> from 'path' >>> + * that means either >>> + * a. The same device disappeared and reappeared with >>> different >>> + * name. or >>> + * b. The missing-disk-which-was-replaced, has >>> reappeared now. >>> + * >>> + * We must allow 1 and 2a above. But 2b would be a >>> spurious and >>> + * unintentional. >>> + * Further in case of 1 and 2a above, the disk at 'path' >>> would have >>> + * missed some transaction when it was away and in case of 2a >>> + * the stale bdev has to be updated as well. >>> + * 2b must not be allowed at all time. >>> + */ >>> + >>> + /* >>> + * As of now don't allow update to btrfs_fs_device through the >>> + * btrfs dev scan cli, after FS has been mounted. >>> + */ >>> + >>> + if (fs_devices->opened) { >>> + printk(KERN_INFO "BTRFS: device list update failed, FS is >>> open"); >>> + return -EBUSY; >>> + } >>> + >> The 'if(fs_devices->opened)' block is in both branch of the >> 'if(!device)' judgement, >> it would be better to extract the code block out of the 'if(!device)' >> block. > > thanks for the comments Qu. > > we have else if. that is when device is found and path is NOT new > (matches with the old) then we shall return success. > > Anand Oh, my bad. Forgot the code can continue without hitting either branch. Thanks, Qu > >> Thanks, >> Qu >>> name = rcu_string_strdup(path, GFP_NOFS); >>> if (!name) >>> return -ENOMEM; >> >> -- >> To unsubscribe from this list: send the line "unsubscribe >> linux-btrfs" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html