| Message ID | 20191007094515.925-1-anand.jain@oracle.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | btrfs: fix issues due to alien device | expand |
On Mon, Oct 07, 2019 at 05:45:10PM +0800, Anand Jain wrote: > v3: Fix alien device is due to wipefs in Patch4. > Fix a nit in Patch3. > Patches are reordered. > > Alien device is a device in fs_devices list having a different fsid than > the expected fsid or no btrfs_magic. This patch set fixes issues found due > to the same. The definition of alien device should be in some of the patches, I see it only in the cover letter. So the sequence of actions mkfs A mount A mkfs B C add B to A mount C leaves the scanned devices in a state that does not match the reality. At the time when B is scanned again, the ownership in the in-memory structures should be transferred to A (ie. removing B from BC). So far I understand the problem. The fix I'd expect is to fix up the devices at the first occasion, like when the device is scanned or attempted for mount. > Patch1: is a cleanup patch, not related. > Patch2: fixes failing to mount a degraded RAIDs (RAID1/5/6/10), by > hardening the function btrfs_free_extra_devids(). > Patch3: fixes the missing device (due to alien btrfs-device) not missing in > the userland, by hardening the function btrfs_open_one_device(). > Patch4: fixes the missing device (due to alien device) not missing in > the userland, by returning EUCLEAN in btrfs_read_dev_one_super(). > Patch5: eliminates the source of the alien device in the fs_devices. I'm a bit lost in the way it's being fixed. The userspace part is IMO irrelevant, the change must happen on the kernel side using the information provided (scan, mount). The error conditions should be propagated in a more fine grained way, similar to what you propose with EUCLEAN, but not with EUCLEAN. That has a very specific meaning, as has been pointed out. The distinctions should be like: < 0 - error 0 - all ok, take the device > 0 - device ok, but not ours And the callers will decide what to do (remove or ignore). > PS: Fundamentally its wrong approach that btrfs-progs deduces the device > missing state in the userland instead of obtaining it from the kernel. > I remember objecting on the btrfs-progs patch which did that, but still > it got merged, bugs in p3 and p4 are its side effects. I wrote > patches to read device_state from the kernel using ioctl, procfs and > sysfs but it didn't get the due attention till a merger. The state has to be ultimately decided by kernel, userspace can read the information from anything but at the time this gets processed, it might be stale again. It's been probably very long ago when the above was discussed and I don't recall the details, it may be a good idea to revisit if there are still things to address.
On 10/8/19 1:36 AM, David Sterba wrote: > On Mon, Oct 07, 2019 at 05:45:10PM +0800, Anand Jain wrote: >> v3: Fix alien device is due to wipefs in Patch4. >> Fix a nit in Patch3. >> Patches are reordered. >> >> Alien device is a device in fs_devices list having a different fsid than >> the expected fsid or no btrfs_magic. This patch set fixes issues found due >> to the same. > > The definition of alien device should be in some of the patches, I see > it only in the cover letter. Ok will include. > So the sequence of actions > > mkfs A > mount A > mkfs B C > add B to A > mount C Right (fixed in patch 3/5). B contains alien btrfs superblock. Another scenario (fixed in Patch 4/5) as below. mkfs A B wipe A OR mkfs.ext4 A mount B 'A' contains alien or no superblock. > leaves the scanned devices in a state that does not match the reality. > At the time when B is scanned again, the ownership in the in-memory > structures should be transferred to A (ie. removing B from BC). So far I > understand the problem. > The fix I'd expect is to fix up the devices at the first occasion, like > when the device is scanned or attempted for mount. Right. btrfs_free_stale_devices() does it. Checks for duplicate entries for a device, and deletes it as its new structure has already been created. But we didn't call this in the context of device add, now fixed in patch 5/5. >> Patch1: is a cleanup patch, not related. >> Patch2: fixes failing to mount a degraded RAIDs (RAID1/5/6/10), by >> hardening the function btrfs_free_extra_devids(). >> Patch3: fixes the missing device (due to alien btrfs-device) not missing in >> the userland, by hardening the function btrfs_open_one_device(). >> Patch4: fixes the missing device (due to alien device) not missing in >> the userland, by returning EUCLEAN in btrfs_read_dev_one_super(). >> Patch5: eliminates the source of the alien device in the fs_devices. > > I'm a bit lost in the way it's being fixed. We weren't checking if there is any stale device structure when a device comes into btrfs through btrfs device add. This patch fixes it. Similar to what we do during scan. These are the only two ways device can entry into the btrfs kernel. > The userspace part is IMO > irrelevant, the change must happen on the kernel side using the > information provided (scan, mount). > > The error conditions should be propagated in a more fine grained way, > similar to what you propose with EUCLEAN, but not with EUCLEAN. That has > a very specific meaning, as has been pointed out. > > The distinctions should be like: > > < 0 - error > 0 - all ok, take the device > > 0 - device ok, but not ours When device is scanned its SB is already been read and updated in fs_devices. Now when we try to mount the SB is found to be corrupted or wiped or alienated. IMO it should be < 0. Its not about device its about the SB (on-disk struct) in the device, so I wonder which one is better -EUCLEAN ? Or -ESTALE ? or any suggestion? > And the callers will decide what to do (remove or ignore). > >> PS: Fundamentally its wrong approach that btrfs-progs deduces the device >> missing state in the userland instead of obtaining it from the kernel. >> I remember objecting on the btrfs-progs patch which did that, but still >> it got merged, bugs in p3 and p4 are its side effects. I wrote >> patches to read device_state from the kernel using ioctl, procfs and >> sysfs but it didn't get the due attention till a merger. > > The state has to be ultimately decided by kernel, userspace can read the > information from anything but at the time this gets processed, Right. > it might > be stale again. That's ok. The user will run the command again. > It's been probably very long ago when the above was > discussed and I don't recall the details, > it may be a good idea to > revisit if there are still things to address. Ok thanks. The correct thread to respond on this is the readmirror thread. As we need to read the btrfs_device::dev_state from the kernel to support readmirror. Thanks, Anand
v3: Fix alien device is due to wipefs in Patch4. Fix a nit in Patch3. Patches are reordered. Alien device is a device in fs_devices list having a different fsid than the expected fsid or no btrfs_magic. This patch set fixes issues found due to the same. Patch1: is a cleanup patch, not related. Patch2: fixes failing to mount a degraded RAIDs (RAID1/5/6/10), by hardening the function btrfs_free_extra_devids(). Patch3: fixes the missing device (due to alien btrfs-device) not missing in the userland, by hardening the function btrfs_open_one_device(). Patch4: fixes the missing device (due to alien device) not missing in the userland, by returning EUCLEAN in btrfs_read_dev_one_super(). Patch5: eliminates the source of the alien device in the fs_devices. PS: Fundamentally its wrong approach that btrfs-progs deduces the device missing state in the userland instead of obtaining it from the kernel. I remember objecting on the btrfs-progs patch which did that, but still it got merged, bugs in p3 and p4 are its side effects. I wrote patches to read device_state from the kernel using ioctl, procfs and sysfs but it didn't get the due attention till a merger. Anand Jain (5): btrfs: drop useless goto in open_fs_devices btrfs: include non-missing as a qualifier for the latest_bdev btrfs: remove identified alien btrfs device in open_fs_devices btrfs: remove identified alien device in open_fs_devices btrfs: free alien device due to device add fs/btrfs/disk-io.c | 2 +- fs/btrfs/volumes.c | 57 +++++++++++++++++++++++++++++++++++++++--------------- 2 files changed, 42 insertions(+), 17 deletions(-)