On Tue, Jul 12, 2022 at 10:29 PM Eric Wheeler <bcache@xxxxxxxxxxxxxxxxxx> wrote: > > On Thu, 7 Jul 2022, Andrea Tomassetti wrote: > > On Wed, Jul 6, 2022 at 12:03 AM Eric Wheeler <bcache@xxxxxxxxxxxxxxxxxx> wrote: > > > On Tue, 5 Jul 2022, Coly Li wrote: > > > > > 2022年7月5日 16:48，Andrea Tomassetti <andrea.tomassetti-opensource@xxxxxxxx> 写道： > > > > > On Mon, Jul 4, 2022 at 5:29 PM Coly Li <colyli@xxxxxxx> wrote: > > > > >>> 2022年7月4日 23:13，Andrea Tomassetti <andrea.tomassetti-opensource@xxxxxxxx> 写道： > > > > >>> Introducing a bcache control device (/dev/bcache_ctrl) that allows > > > > >>> communicating to the driver from user space via IOCTL. The only > > > > >>> IOCTL commands currently implemented, receives a struct cache_sb and > > > > >>> uses it to register the backing device without any need of > > > > >>> formatting them. > > > > >>> > > > > >> Back to the patch, I don’t support this idea. For the problem you are > > > > >> solving, indeed people uses device mapper linear target for many > > > > >> years, and it works perfectly without any code modification. > > > > >> > > > > >> That is, create a 8K image and set it as a loop device, then write a > > > > >> dm table to combine it with the existing hard drive. Then you run > > > > >> “bcache make -B <combined dm target>”, you will get a bcache device > > > > >> whose first 8K in the loop device and existing super block of the > > > > >> hard driver located at expected offset. > > > > >> > > > > > We evaluated this option but we weren't satisfied by the outcomes for, > > > > > at least, two main reasons: complexity and operational drawbacks. For > > > > > the complexity side: in production we use more than 20 HD that need to > > > > > be cached. It means we need to create 20+ header for them, 20+ loop > > > > > devices and 20+ dm linear devices. So, basically, there's a 3x factor > > > > > for each HD that we want to cache. Moreover, we're currently using > > > > > ephemeral disks as cache devices. In case of a machine reboot, > > > > > ephemeral devices can get lost; at this point, there will be some > > > > > trouble to mount the dm-linear bcache backing device because there > > > > > will be no cache device. > > > > > > > > OK, I get your point. It might make sense for your situation, although I > > > > know some other people use the linear dm-table to solve similar > > > > situation but may be not perfect for you. This patch may work in your > > > > procedure, but there are following things make me uncomfortable, > > > > > > Coly is right: there are some issues to address. > > > > > > Coly, I do not wish to contradict you, only to add that we have had times > > > where this would be useful. I like the idea, particularly avoiding placing > > > dm-linear atop of the bdev which reduces the IO codepath. Maybe there is > > > an implementation that would fit everyone's needs. > > > > > > For us, such a superblock-less registration could resolve two issues: > > > > > > 1. Most commonly we wish to add bcache to an existing device without > > > re-formatting and without adding the dm-linear complexity. > > > > That's exactly what was preventing us from using bcache in production > > prior to this patch. > > Ok, but we always use writeback...and others may wish to, too. I think > any patch that introduces a feature needs to support existing features > without introducing limitations on the behavior. > Totally agree. My only point was that I extensively tested this patch with wt mode. It works in wb mode as well, for sure because the backing device's header is almost never used. The only issue I can foresee in wb mode is in case of a machine reboot: the backing device will lose the virtual header and, at boot time, another one will be generated. It will get attached again to its cache device with a new UID and I'm not sure if this will imply the loss of the data that was not previously written to it, but was only present on the cache device. But I think that losing data it's a well-known risk of wb mode. > > > 2. Relatedly, IMHO, I've never liked the default location at 8k because we > > > like to align our bdev's to the RAID stride width and rarely is the > > > bdev array aligned at 8k (usually 64k or 256k for hardware RAID). If > > > we forget to pass the --data-offset at make-bcache time then we are > > > stuck with an 8k-misalignment for the life of the device. > > > > > > In #2 we usually reformat the volume to avoid dm-linear complexity (and in > > > both cases we have wanted writeback cache support). This process can take > > > a while to `dd` ‾30TBs of bdev on spinning disks and we have to find > > > temporary disk space to move that data out and back in. > > > > > > > - Copying a user space memory and directly using it as a complicated in-kernel memory object. > > > > > > In the ioctl changes for bch_write_bdev_super() there does not appear to > > > be a way to handle attaching caches to the running bcache. For example: > > > > > > 1. Add a cacheless bcache0 volume via ioctl > > > 2. Attach a writeback cache, write some data. > > > 3. Unregister everything > > > 4. Re-register the _dirty_ bdev from #1 > > > > > > In this scenario bcache will start "running" immediately because it > > > cannot update its cset.uuid (as reported by bcache-super-show) which I > > > believe is stored in cache_sb.set_uuid. > > > > > > I suppose in step #2 you could update your userspace state with the > > > cset.uuid so your userspace ioctl register tool would put the cset.uuid in > > > place, but this is fragile without good userspace integration. > > > > > > I could imagine something like /etc/bcachetab (like crypttab or > > > mdadm.conf) that maps cset UUID's to bdev UUIDs. Then your userspace > > > ioctl tool could be included in a udev script to auto-load volumes as they > > > become available. > > Yes, in conjunction with this patch, I developed a python udev script > > that manages device registration base on a YAML configuration file. I > > even patched bcache-tools to support the new IOCTL registration. You > > will find a link to the Github project at the end of this message. > > Fewer dependencies are better: There are still python2 vs python3 > conflicts out there---and loading python and its dependencies into an > initrd/initramfs for early bcache loading could be very messy, indeed! > > You've already put some work into make-bcache so creating a bcache_udev > function and a bcache-udev.c file (like make-bcache.c) is probably easy > enough. IMHO, a single-line grepable format (instead of YAML) could be > used to minimize dependencies so that it is clean in an initramfs in the > same way that mdadm.conf and crypttab already do. You could then parse it > with C or bash pretty easily... > I will be really glad to rework the patch if we can agree on some modifications that will make it suitable to be merged upstream. > > > You want to make sure that when a dirty writeback-cached bdev is > > > registered that it does not activate until either: > > > > > > 1. The cache is attached > > > 2. echo 1 > /sys/block/bcache0/bcache/running > > > > > > > - A new IOCTL interface added, even all rested interface are sysfs based. > > > > > > True. Perhaps something like this would be better, and it would avoid > > > sharing a userspace page for blindly populating `struct cache_sb`, too: > > > > > > echo '/dev/bdev [bdev_uuid] [/dev/cachedev|cset_uuid] [options]' > ¥ > > > /sys/fs/bcache/register_raw > > > > I thought that introducing an IOCTL interface could be a stopper for > > this patch, so I'm more than welcome to refactor it using sysfs > > entries only. But, I will do so only if there's even the slightest > > chance that this patch can be merged. > > I'm for the patch with sysfs, but I'm not the decision maker either. > As Eric already asked, > Coly: what would your requirements be to accept this upstream? > Andrea > > > Because of the writeback issue above, the cache and bdev either need to be > > > registered simultaneously or the cset uuid need to be specified. > > > > > > > - Do things in kernel space while there is solution in user space. > > > > > > > > All the above opinions are quite personal to myself, I don’t say you are > > > > wrong or I am correct. If the patch works, that’s cool and you can use > > > > it as you want, but I don’t support it to be in upstream. > > > > > > An alternate implementation might be to create a dm-bcache target. The > > > core bcache code could be left alone except a few EXPORT_SYMBOL()'s so > > > dm-bcache can reach the bcache registration bits. > > > > > > This would: > > > * Minimally impact the bcache code base > > > * Solve the blind-populating of `struct cache_sb` issue in the same way > > > as `register_raw` could work above. > > > * Create a nicely segmented codebase (dm target) to upstream separately > > > through the DM team. > > > * Could be maintained cleanly out-of-tree because the bcache > > > registration interfaces change very infrequently. > > > * bdev resize could be done with a `dmsetup replace` but you'll need to > > > add resize support as below. > > > > > > > > For the operational drawbacks: from time to time, we exploit the > > > > > online filesystem resize capability of XFS to increase the volume > > > > > size. This would be at least tedious, if not nearly impossible, if the > > > > > volume is mapped inside a dm-linear. > > > > > > > > Currently bcache doesn’t support cache or backing device resize. I don’t > > > > see the connection between above statement and feeding an in-memory > > > > super block via IOCTL command. > > > > > > Resize is perhaps unrelated, so if you decide to tackle bdev or cachedev > > > resize then please start a new list thread. Briefly: supporting bdev > > > resize is probably easy. I've looked through the code a few times with > > > this in mind but haven't had time. > > > > > > Here's the summary, not sure if there are any other > > > considerations: > > > > > > 1. Create a sysfs file like /sys/block/bcache0/bcache/resize to trigger > > > resize on echo 1 >. > > > 2. Refactor the set_capacity() bits from bcache_device_init() into its > > > own function. > > > 3. Put locks around bcache_device.full_dirty_stripes and > > > bcache_device.stripe_sectors_dirty. Re-alloc+copy+free and zero the > > > new bytes at the end. > > > > > > The cachedev's don't know anything about the bdev size, so (according to > > > Kent) they will "just work" by referencing new offsets that didn't exist > > > before when IOs come through. (This is basically the same as resizing the > > > bdev and then unregister/re-register which is how we resize bdevs now.) > > > > > > As for resizing a cachedev, I've not looked at all---not sure about that > > > one. We always detach, resize, make-bcache and re-attach the new cache. > > > Maybe it is similarly simple, but haven't looked. > > > > > > > >> It is unnecessary to create a new IOCTL interface, and I feel the way > > > > >> how it works is really unconvinced for potential security risk. > > > > >> > > > > > Which potential security risks concern you? > > > > > > > > > > > > > Currently we don’t check all members of struct cache_sb_disk, what we do > > > > is to simply trust bcache-tools. Create a new IOCTL interface to send a > > > > user space memory into kernel space as superblock, that needs quite a > > > > lot of checking code to make sure it won’t panic the kernel. It is > > > > possible, but it is not worthy to add so many code for the purpose, > > > > especially adding them into upstream code. > > > > > > > > I am not able to provide an explicit example for security risk, just the > > > > new adding interface makes me not so confident. > > > > > > Maintaining a blind structure population from a userspace page would be > > > difficult as well because even if the kernel validates _everything_ in > > > cache_sb today, anyone extending `struct cache_sb` needs to remember to > > > add checks for that. Failing to enforce validation inside the kernel could > > > lead to kernel crashes or data corruption from userspace which is of > > > course never good. > > > > > > > Can I solve this by refactoring the patch and using sysfs based > > registration instead? > > Sure, just make the sysfs interface configurable enough that: > - all configuration is text (not binary) > - bcache.txt documentation is updated > > -Eric > > > For anyone that is interested to try this solution, and as a > > reference, I leave here below the links to my public Github > > repositories: > > - bcache-tools: https://github.com/andreatomassetti/bcache-tools > > - bcache (standalone, patched): https://github.com/andreatomassetti/bcache > > > > Thank you very much, > > Andrea > > > > > We always assume that, somehow, someone could leverage an insecure IOCTL > > > call and crash the OS when they shouldn't be allowed to (even if they are > > > not root, like from sudo). This is a security issue from an assurance and > > > integrity point of view, even if there might be neither an obvious > > > privelege escalation nor privacy concern. > > > > > > -Eric > > > > > > > > > > Thanks. > > > > > > > > Coly Li > > > > > > > > > > > > > > > > > >
