| Message ID | 20230126033358.1880-1-demi@invisiblethingslab.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Allow race-free block device handling | expand |
On Wed, Jan 25 2023 at 10:33P -0500, Demi Marie Obenour <demi@invisiblethingslab.com> wrote: > This work aims to allow userspace to create and destroy block devices > in a race-free and leak-free way, "race-free and leak-free way" implies there both races and leaks in existing code. You're making claims that are likely very specific to your Xen use-case. Please explain more carefully. > and to allow them to be exposed to > other Xen VMs via blkback without leaks or races. It’s marked as RFC > for a few reasons: > > - The code has been only lightly tested. It might be unstable or > insecure. > > - The DM_DEV_CREATE ioctl gains a new flag. Unknown flags were > previously ignored, so this could theoretically break buggy userspace > tools. Not seeing a reason that type of DM change is needed. If you feel strongly about it send a separate patch and we can discuss it. > - I have no idea if I got the block device reference counting and > locking correct. Your headers and justifcation for this line of work are really way too terse. Please take the time to clearly make the case for your changes in both the patch headers and code. Mike -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Thu, Feb 02, 2023 at 11:50:37AM -0500, Mike Snitzer wrote: > On Wed, Jan 25 2023 at 10:33P -0500, > Demi Marie Obenour <demi@invisiblethingslab.com> wrote: > > > This work aims to allow userspace to create and destroy block devices > > in a race-free and leak-free way, > > "race-free and leak-free way" implies there both races and leaks in > existing code. You're making claims that are likely very specific to > your Xen use-case. Please explain more carefully. Will do in v2. > > and to allow them to be exposed to > > other Xen VMs via blkback without leaks or races. It’s marked as RFC > > for a few reasons: > > > > - The code has been only lightly tested. It might be unstable or > > insecure. > > > > - The DM_DEV_CREATE ioctl gains a new flag. Unknown flags were > > previously ignored, so this could theoretically break buggy userspace > > tools. > > Not seeing a reason that type of DM change is needed. If you feel > strongly about it send a separate patch and we can discuss it. Patch 2/7 is the diskseq change. v2 will contain a revised and tested version with a greatly expanded commit message. > > - I have no idea if I got the block device reference counting and > > locking correct. > > Your headers and justifcation for this line of work are really way too > terse. Please take the time to clearly make the case for your changes > in both the patch headers and code. I will expand the commit message in v2, but I am not sure what you want me to add to the code comments. Would you mind explaining?
On Thu, Feb 02 2023 at 1:41P -0500, Demi Marie Obenour <demi@invisiblethingslab.com> wrote: > On Thu, Feb 02, 2023 at 11:50:37AM -0500, Mike Snitzer wrote: > > On Wed, Jan 25 2023 at 10:33P -0500, > > Demi Marie Obenour <demi@invisiblethingslab.com> wrote: > > > > > This work aims to allow userspace to create and destroy block devices > > > in a race-free and leak-free way, > > > > "race-free and leak-free way" implies there both races and leaks in > > existing code. You're making claims that are likely very specific to > > your Xen use-case. Please explain more carefully. > > Will do in v2. > > > > and to allow them to be exposed to > > > other Xen VMs via blkback without leaks or races. It’s marked as RFC > > > for a few reasons: > > > > > > - The code has been only lightly tested. It might be unstable or > > > insecure. > > > > > > - The DM_DEV_CREATE ioctl gains a new flag. Unknown flags were > > > previously ignored, so this could theoretically break buggy userspace > > > tools. > > > > Not seeing a reason that type of DM change is needed. If you feel > > strongly about it send a separate patch and we can discuss it. > > Patch 2/7 is the diskseq change. v2 will contain a revised and tested > version with a greatly expanded commit message. I'm aware that 2/7 is where you make the DM change to disallow unknown flags, what I'm saying is I don't see a reason for that change. Certainly doesn't look to be a requirement for everything else in that patch. So send a separate patch, but I'm inclined to _not_ accept it because it does potentially break some userspace. > > > - I have no idea if I got the block device reference counting and > > > locking correct. > > > > Your headers and justifcation for this line of work are really way too > > terse. Please take the time to clearly make the case for your changes > > in both the patch headers and code. > > I will expand the commit message in v2, but I am not sure what you want > me to add to the code comments. Would you mind explaining? Nothing specific about code, was just a general reminder (based on how terse the 2/7 header was). Mike -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Thu, Feb 02, 2023 at 02:56:34PM -0500, Mike Snitzer wrote: > On Thu, Feb 02 2023 at 1:41P -0500, > Demi Marie Obenour <demi@invisiblethingslab.com> wrote: > > > On Thu, Feb 02, 2023 at 11:50:37AM -0500, Mike Snitzer wrote: > > > On Wed, Jan 25 2023 at 10:33P -0500, > > > Demi Marie Obenour <demi@invisiblethingslab.com> wrote: > > > > > > > This work aims to allow userspace to create and destroy block devices > > > > in a race-free and leak-free way, > > > > > > "race-free and leak-free way" implies there both races and leaks in > > > existing code. You're making claims that are likely very specific to > > > your Xen use-case. Please explain more carefully. > > > > Will do in v2. > > > > > > and to allow them to be exposed to > > > > other Xen VMs via blkback without leaks or races. It’s marked as RFC > > > > for a few reasons: > > > > > > > > - The code has been only lightly tested. It might be unstable or > > > > insecure. > > > > > > > > - The DM_DEV_CREATE ioctl gains a new flag. Unknown flags were > > > > previously ignored, so this could theoretically break buggy userspace > > > > tools. > > > > > > Not seeing a reason that type of DM change is needed. If you feel > > > strongly about it send a separate patch and we can discuss it. > > > > Patch 2/7 is the diskseq change. v2 will contain a revised and tested > > version with a greatly expanded commit message. > > I'm aware that 2/7 is where you make the DM change to disallow unknown > flags, what I'm saying is I don't see a reason for that change. Thanks for the clarification. > Certainly doesn't look to be a requirement for everything else in that > patch. Indeed it is not. I will make it a separate patch. > So send a separate patch, but I'm inclined to _not_ accept it because > it does potentially break some userspace. Is it okay to add DM_FILE_DESCRIPTOR_FLAG (with the same meaning as in 2/7) _without_ rejecting unknown flags? The same patch would bump the minor version number, so userspace would still be able to tell if the kernel supported DM_FILE_DESCRIPTOR_FLAG. If you wanted, I could ignore DM_FILE_DESCRIPTOR_FLAG unless the minor number passed by userspace is sufficiently recent. Another option would be to make userspace opt-in to strict parameter checking by passing 5 as the major version instead of 4. Userspace programs that passed 4 would get the old behavior, while userspace programs that passed 5 would get strict parameter checking and be able to use new features such as DM_FILE_DESCRIPTOR_FLAG. > > > > - I have no idea if I got the block device reference counting and > > > > locking correct. > > > > > > Your headers and justifcation for this line of work are really way too > > > terse. Please take the time to clearly make the case for your changes > > > in both the patch headers and code. > > > > I will expand the commit message in v2, but I am not sure what you want > > me to add to the code comments. Would you mind explaining? > > Nothing specific about code, was just a general reminder (based on how > terse the 2/7 header was). > > Mike Thanks for the feedback!