| Message ID | 20180719034118.27741-1-famz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/18/2018 11:41 PM, Fam Zheng wrote: > On my Fedora 28, /dev/null is locked by some other process (couldn't > inspect it due to the current lslocks limitation), so iotests 226 fails > with some unexpected image locking errors because it uses qemu-io to > open it. > > Actually it's safe to not use any lock on /dev/null or /dev/zero. > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > block/file-posix.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/block/file-posix.c b/block/file-posix.c > index 60af4b3d51..8bf034108a 100644 > --- a/block/file-posix.c > +++ b/block/file-posix.c > @@ -503,7 +503,12 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, > s->use_lock = false; > break; > case ON_OFF_AUTO_AUTO: > - s->use_lock = qemu_has_ofd_lock(); > + if (!strcmp(filename, "/dev/null") || > + !strcmp(filename, "/dev/zero")) { > + s->use_lock = false; > + } else { > + s->use_lock = qemu_has_ofd_lock(); > + } > break; > default: > abort(); > I apparently caused a lot of failures with 226, oops! Should we instead modify the test in this case to not attempt to take a lock on a device we know cannot meaningfully store state, or is it your preference to attempt to maintain such a list in the raw driver itself? I guess we never want QEMU to try to lock things like /dev/zero, but I don't know if there are more such pseudo-devices we should never try to lock and if so, what common property unifies them such that we don't have to maintain a list.
On Thu, 07/19 13:57, John Snow wrote: > Should we instead modify the test in this case to not attempt to take a > lock on a device we know cannot meaningfully store state, or is it your > preference to attempt to maintain such a list in the raw driver itself? > > I guess we never want QEMU to try to lock things like /dev/zero, but I > don't know if there are more such pseudo-devices we should never try to > lock and if so, what common property unifies them such that we don't > have to maintain a list. They are 0 sized anyway so I only expect them to be used in test cases like the one we're fixing. So this really doesn't matter. An exceptional one would be /dev/nullb* but that is not used in real world either. I'm not trying to maintain a complete list (e.g. /dev/urandom and /dev/nullb* are missing), this patch is just trying to make writing test code easier. Fam
On 07/19/2018 12:57 PM, John Snow wrote: > > Should we instead modify the test in this case to not attempt to take a > lock on a device we know cannot meaningfully store state, or is it your > preference to attempt to maintain such a list in the raw driver itself? > > I guess we never want QEMU to try to lock things like /dev/zero, but I > don't know if there are more such pseudo-devices we should never try to > lock and if so, what common property unifies them such that we don't > have to maintain a list. One potential common property: /dev/zero and /dev/null are character devices, rather than block devices. Character devices in general don't make much sense for holding stateful images, so it may be as simple as gating our locking based on fstat() distinguishing which type of file we are accessing.
On 07/20/2018 03:24 AM, Fam Zheng wrote: > On Thu, 07/19 13:57, John Snow wrote: >> Should we instead modify the test in this case to not attempt to take a >> lock on a device we know cannot meaningfully store state, or is it your >> preference to attempt to maintain such a list in the raw driver itself? >> >> I guess we never want QEMU to try to lock things like /dev/zero, but I >> don't know if there are more such pseudo-devices we should never try to >> lock and if so, what common property unifies them such that we don't >> have to maintain a list. > > They are 0 sized anyway so I only expect them to be used in test cases like the > one we're fixing. So this really doesn't matter. An exceptional one would be > /dev/nullb* but that is not used in real world either. I'm not familiar with /dev/nullb* - is that a typo? $ ll /dev/nullb* ls: cannot access '/dev/nullb*': No such file or directory > > I'm not trying to maintain a complete list (e.g. /dev/urandom and /dev/nullb* > are missing), this patch is just trying to make writing test code easier. /dev/urandom is also a character device, and also fits in my heuristic that we likely only care about locking of block devices.
On 07/20/2018 04:24 AM, Fam Zheng wrote: > On Thu, 07/19 13:57, John Snow wrote: >> Should we instead modify the test in this case to not attempt to take a >> lock on a device we know cannot meaningfully store state, or is it your >> preference to attempt to maintain such a list in the raw driver itself? >> >> I guess we never want QEMU to try to lock things like /dev/zero, but I >> don't know if there are more such pseudo-devices we should never try to >> lock and if so, what common property unifies them such that we don't >> have to maintain a list. > > They are 0 sized anyway so I only expect them to be used in test cases like the > one we're fixing. So this really doesn't matter. An exceptional one would be > /dev/nullb* but that is not used in real world either. > I agree in general; I'm questioning somewhat if we ought to just patch the test instead of the driver, given that we can't really be consistent or accurate about when we decide not to take the lock in the driver; unless we use something like Eric's suggestion (we don't take locks on char devices, maybe?) > I'm not trying to maintain a complete list (e.g. /dev/urandom and /dev/nullb* > are missing), this patch is just trying to make writing test code easier. > Yeah, it's the little quality-of-life band-aid that I'm wondering if we ought to stick in here. Granted, this fixes a test that's broken, so... Reviewed-by: John Snow <jsnow@redhat.com> I'll let Kevin decide if we ought to patch it nicer than this. --js > Fam >
On Sat, Jul 21, 2018 at 12:35 AM Eric Blake <eblake@redhat.com> wrote: > > On 07/20/2018 03:24 AM, Fam Zheng wrote: > I'm not familiar with /dev/nullb* - is that a typo? > > $ ll /dev/nullb* > ls: cannot access '/dev/nullb*': No such file or directory You probably have figured out already but just in case: it's kernel null_blk mod. Fam
On 2018-07-19 05:41, Fam Zheng wrote: > On my Fedora 28, /dev/null is locked by some other process (couldn't > inspect it due to the current lslocks limitation), so iotests 226 fails > with some unexpected image locking errors because it uses qemu-io to > open it. > > Actually it's safe to not use any lock on /dev/null or /dev/zero. > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > block/file-posix.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/block/file-posix.c b/block/file-posix.c > index 60af4b3d51..8bf034108a 100644 > --- a/block/file-posix.c > +++ b/block/file-posix.c > @@ -503,7 +503,12 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, > s->use_lock = false; > break; > case ON_OFF_AUTO_AUTO: > - s->use_lock = qemu_has_ofd_lock(); > + if (!strcmp(filename, "/dev/null") || > + !strcmp(filename, "/dev/zero")) { I’m not sure I like a strcmp() based on filename (though it should work for all of the cases where someone would want to specify either of those for a qemu block device). Isn’t there some specific major/minor number we can use to check for these two files? Or are those Linux-specific? I could also imagine just not locking any host_device, but since people do use qcow2 immediately on block devices, maybe we do want to lock them. Finally, if really all you are trying to do is to make test code easier, then I don’t know exactly why. Just disabling locking in 226 shouldn’t be too hard. Max > + s->use_lock = false; > + } else { > + s->use_lock = qemu_has_ofd_lock(); > + } > break; > default: > abort(); >
On Sun, Jul 22, 2018 at 5:08 AM Max Reitz <mreitz@redhat.com> wrote: > > On 2018-07-19 05:41, Fam Zheng wrote: > > On my Fedora 28, /dev/null is locked by some other process (couldn't > > inspect it due to the current lslocks limitation), so iotests 226 fails > > with some unexpected image locking errors because it uses qemu-io to > > open it. > > > > Actually it's safe to not use any lock on /dev/null or /dev/zero. > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > --- > > block/file-posix.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/block/file-posix.c b/block/file-posix.c > > index 60af4b3d51..8bf034108a 100644 > > --- a/block/file-posix.c > > +++ b/block/file-posix.c > > @@ -503,7 +503,12 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, > > s->use_lock = false; > > break; > > case ON_OFF_AUTO_AUTO: > > - s->use_lock = qemu_has_ofd_lock(); > > + if (!strcmp(filename, "/dev/null") || > > + !strcmp(filename, "/dev/zero")) { > > I’m not sure I like a strcmp() based on filename (though it should work > for all of the cases where someone would want to specify either of those > for a qemu block device). Isn’t there some specific major/minor number > we can use to check for these two files? Or are those Linux-specific? Yeah, I guess major/minor numbers are Linux-specific. > > I could also imagine just not locking any host_device, but since people > do use qcow2 immediately on block devices, maybe we do want to lock them. That's right. > > Finally, if really all you are trying to do is to make test code easier, > then I don’t know exactly why. Just disabling locking in 226 shouldn’t > be too hard. The tricky thing is in remembering to do that for future test cases. My machine seems to be somehow different than John's so that my 226 cannot lock /dev/null, but I'm not sure that is the case for other releases, distros or system instances. Fam > > Max > > > + s->use_lock = false; > > + } else { > > + s->use_lock = qemu_has_ofd_lock(); > > + } > > break; > > default: > > abort(); > > > >
On 2018-07-22 04:37, Fam Zheng wrote: > On Sun, Jul 22, 2018 at 5:08 AM Max Reitz <mreitz@redhat.com> wrote: >> >> On 2018-07-19 05:41, Fam Zheng wrote: >>> On my Fedora 28, /dev/null is locked by some other process (couldn't >>> inspect it due to the current lslocks limitation), so iotests 226 fails >>> with some unexpected image locking errors because it uses qemu-io to >>> open it. >>> >>> Actually it's safe to not use any lock on /dev/null or /dev/zero. >>> >>> Signed-off-by: Fam Zheng <famz@redhat.com> >>> --- >>> block/file-posix.c | 7 ++++++- >>> 1 file changed, 6 insertions(+), 1 deletion(-) >>> >>> diff --git a/block/file-posix.c b/block/file-posix.c >>> index 60af4b3d51..8bf034108a 100644 >>> --- a/block/file-posix.c >>> +++ b/block/file-posix.c >>> @@ -503,7 +503,12 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, >>> s->use_lock = false; >>> break; >>> case ON_OFF_AUTO_AUTO: >>> - s->use_lock = qemu_has_ofd_lock(); >>> + if (!strcmp(filename, "/dev/null") || >>> + !strcmp(filename, "/dev/zero")) { >> >> I’m not sure I like a strcmp() based on filename (though it should work >> for all of the cases where someone would want to specify either of those >> for a qemu block device). Isn’t there some specific major/minor number >> we can use to check for these two files? Or are those Linux-specific? > > Yeah, I guess major/minor numbers are Linux-specific. > >> >> I could also imagine just not locking any host_device, but since people >> do use qcow2 immediately on block devices, maybe we do want to lock them. > > That's right. > >> >> Finally, if really all you are trying to do is to make test code easier, >> then I don’t know exactly why. Just disabling locking in 226 shouldn’t >> be too hard. > > The tricky thing is in remembering to do that for future test cases. > My machine seems to be somehow different than John's so that my 226 > cannot lock /dev/null, but I'm not sure that is the case for other > releases, distros or system instances. Usually we don’t need to use /dev/null, though, because null-co:// is better suited for most purposes. This is a very specific test of host devices. Maybe we should start a doc file with common good practices about writing iotests? Max
On Sun, Jul 22, 2018 at 10:06 PM Max Reitz <mreitz@redhat.com> wrote: > > On 2018-07-22 04:37, Fam Zheng wrote: > > On Sun, Jul 22, 2018 at 5:08 AM Max Reitz <mreitz@redhat.com> wrote: > >> > >> On 2018-07-19 05:41, Fam Zheng wrote: > >>> On my Fedora 28, /dev/null is locked by some other process (couldn't > >>> inspect it due to the current lslocks limitation), so iotests 226 fails > >>> with some unexpected image locking errors because it uses qemu-io to > >>> open it. > >>> > >>> Actually it's safe to not use any lock on /dev/null or /dev/zero. > >>> > >>> Signed-off-by: Fam Zheng <famz@redhat.com> > >>> --- > >>> block/file-posix.c | 7 ++++++- > >>> 1 file changed, 6 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/block/file-posix.c b/block/file-posix.c > >>> index 60af4b3d51..8bf034108a 100644 > >>> --- a/block/file-posix.c > >>> +++ b/block/file-posix.c > >>> @@ -503,7 +503,12 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, > >>> s->use_lock = false; > >>> break; > >>> case ON_OFF_AUTO_AUTO: > >>> - s->use_lock = qemu_has_ofd_lock(); > >>> + if (!strcmp(filename, "/dev/null") || > >>> + !strcmp(filename, "/dev/zero")) { > >> > >> I’m not sure I like a strcmp() based on filename (though it should work > >> for all of the cases where someone would want to specify either of those > >> for a qemu block device). Isn’t there some specific major/minor number > >> we can use to check for these two files? Or are those Linux-specific? > > > > Yeah, I guess major/minor numbers are Linux-specific. > > > >> > >> I could also imagine just not locking any host_device, but since people > >> do use qcow2 immediately on block devices, maybe we do want to lock them. > > > > That's right. > > > >> > >> Finally, if really all you are trying to do is to make test code easier, > >> then I don’t know exactly why. Just disabling locking in 226 shouldn’t > >> be too hard. > > > > The tricky thing is in remembering to do that for future test cases. > > My machine seems to be somehow different than John's so that my 226 > > cannot lock /dev/null, but I'm not sure that is the case for other > > releases, distros or system instances. > > Usually we don’t need to use /dev/null, though, because null-co:// is > better suited for most purposes. This is a very specific test of host > devices. > > Maybe we should start a doc file with common good practices about > writing iotests? Yes, mentioning using pseudo devices in docs/devel/testing.rst would probably be a good idea. So is my understanding right that you prefer fixing the test case and discard this patch? If so I'll send another version together with the doc update. Fam > > Max >
On 2018-07-23 03:56, Fam Zheng wrote: > On Sun, Jul 22, 2018 at 10:06 PM Max Reitz <mreitz@redhat.com> wrote: >> >> On 2018-07-22 04:37, Fam Zheng wrote: >>> On Sun, Jul 22, 2018 at 5:08 AM Max Reitz <mreitz@redhat.com> wrote: >>>> >>>> On 2018-07-19 05:41, Fam Zheng wrote: >>>>> On my Fedora 28, /dev/null is locked by some other process (couldn't >>>>> inspect it due to the current lslocks limitation), so iotests 226 fails >>>>> with some unexpected image locking errors because it uses qemu-io to >>>>> open it. >>>>> >>>>> Actually it's safe to not use any lock on /dev/null or /dev/zero. >>>>> >>>>> Signed-off-by: Fam Zheng <famz@redhat.com> >>>>> --- >>>>> block/file-posix.c | 7 ++++++- >>>>> 1 file changed, 6 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/block/file-posix.c b/block/file-posix.c >>>>> index 60af4b3d51..8bf034108a 100644 >>>>> --- a/block/file-posix.c >>>>> +++ b/block/file-posix.c >>>>> @@ -503,7 +503,12 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, >>>>> s->use_lock = false; >>>>> break; >>>>> case ON_OFF_AUTO_AUTO: >>>>> - s->use_lock = qemu_has_ofd_lock(); >>>>> + if (!strcmp(filename, "/dev/null") || >>>>> + !strcmp(filename, "/dev/zero")) { >>>> >>>> I’m not sure I like a strcmp() based on filename (though it should work >>>> for all of the cases where someone would want to specify either of those >>>> for a qemu block device). Isn’t there some specific major/minor number >>>> we can use to check for these two files? Or are those Linux-specific? >>> >>> Yeah, I guess major/minor numbers are Linux-specific. >>> >>>> >>>> I could also imagine just not locking any host_device, but since people >>>> do use qcow2 immediately on block devices, maybe we do want to lock them. >>> >>> That's right. >>> >>>> >>>> Finally, if really all you are trying to do is to make test code easier, >>>> then I don’t know exactly why. Just disabling locking in 226 shouldn’t >>>> be too hard. >>> >>> The tricky thing is in remembering to do that for future test cases. >>> My machine seems to be somehow different than John's so that my 226 >>> cannot lock /dev/null, but I'm not sure that is the case for other >>> releases, distros or system instances. >> >> Usually we don’t need to use /dev/null, though, because null-co:// is >> better suited for most purposes. This is a very specific test of host >> devices. >> >> Maybe we should start a doc file with common good practices about >> writing iotests? > > Yes, mentioning using pseudo devices in docs/devel/testing.rst would > probably be a good idea. So is my understanding right that you prefer > fixing the test case and discard this patch? If so I'll send another > version together with the doc update. I personally would prefer fixing the test and not modifying the code, yes. But I'm aware that it is a personal opinion. I think another alternative would be to not lock character devices, as I don't suppose anyone is going to use them for anything serious. Max
On Mon, Jul 23, 2018 at 10:37 PM Max Reitz <mreitz@redhat.com> wrote: > > On 2018-07-23 03:56, Fam Zheng wrote: > > On Sun, Jul 22, 2018 at 10:06 PM Max Reitz <mreitz@redhat.com> wrote: > >> > >> On 2018-07-22 04:37, Fam Zheng wrote: > >>> On Sun, Jul 22, 2018 at 5:08 AM Max Reitz <mreitz@redhat.com> wrote: > >>>> > >>>> On 2018-07-19 05:41, Fam Zheng wrote: > >>>>> On my Fedora 28, /dev/null is locked by some other process (couldn't > >>>>> inspect it due to the current lslocks limitation), so iotests 226 fails > >>>>> with some unexpected image locking errors because it uses qemu-io to > >>>>> open it. > >>>>> > >>>>> Actually it's safe to not use any lock on /dev/null or /dev/zero. > >>>>> > >>>>> Signed-off-by: Fam Zheng <famz@redhat.com> > >>>>> --- > >>>>> block/file-posix.c | 7 ++++++- > >>>>> 1 file changed, 6 insertions(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/block/file-posix.c b/block/file-posix.c > >>>>> index 60af4b3d51..8bf034108a 100644 > >>>>> --- a/block/file-posix.c > >>>>> +++ b/block/file-posix.c > >>>>> @@ -503,7 +503,12 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, > >>>>> s->use_lock = false; > >>>>> break; > >>>>> case ON_OFF_AUTO_AUTO: > >>>>> - s->use_lock = qemu_has_ofd_lock(); > >>>>> + if (!strcmp(filename, "/dev/null") || > >>>>> + !strcmp(filename, "/dev/zero")) { > >>>> > >>>> I’m not sure I like a strcmp() based on filename (though it should work > >>>> for all of the cases where someone would want to specify either of those > >>>> for a qemu block device). Isn’t there some specific major/minor number > >>>> we can use to check for these two files? Or are those Linux-specific? > >>> > >>> Yeah, I guess major/minor numbers are Linux-specific. > >>> > >>>> > >>>> I could also imagine just not locking any host_device, but since people > >>>> do use qcow2 immediately on block devices, maybe we do want to lock them. > >>> > >>> That's right. > >>> > >>>> > >>>> Finally, if really all you are trying to do is to make test code easier, > >>>> then I don’t know exactly why. Just disabling locking in 226 shouldn’t > >>>> be too hard. > >>> > >>> The tricky thing is in remembering to do that for future test cases. > >>> My machine seems to be somehow different than John's so that my 226 > >>> cannot lock /dev/null, but I'm not sure that is the case for other > >>> releases, distros or system instances. > >> > >> Usually we don’t need to use /dev/null, though, because null-co:// is > >> better suited for most purposes. This is a very specific test of host > >> devices. > >> > >> Maybe we should start a doc file with common good practices about > >> writing iotests? > > > > Yes, mentioning using pseudo devices in docs/devel/testing.rst would > > probably be a good idea. So is my understanding right that you prefer > > fixing the test case and discard this patch? If so I'll send another > > version together with the doc update. > > I personally would prefer fixing the test and not modifying the code, > yes. But I'm aware that it is a personal opinion. Sure, and you are the maintainer, so why not follow what you think. :) Fam
diff --git a/block/file-posix.c b/block/file-posix.c index 60af4b3d51..8bf034108a 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -503,7 +503,12 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, s->use_lock = false; break; case ON_OFF_AUTO_AUTO: - s->use_lock = qemu_has_ofd_lock(); + if (!strcmp(filename, "/dev/null") || + !strcmp(filename, "/dev/zero")) { + s->use_lock = false; + } else { + s->use_lock = qemu_has_ofd_lock(); + } break; default: abort();
On my Fedora 28, /dev/null is locked by some other process (couldn't inspect it due to the current lslocks limitation), so iotests 226 fails with some unexpected image locking errors because it uses qemu-io to open it. Actually it's safe to not use any lock on /dev/null or /dev/zero. Signed-off-by: Fam Zheng <famz@redhat.com> --- block/file-posix.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)