2009-03-25 19:01:08

by Bill Nottingham

[permalink] [raw]
Subject: [PATCH] Add a 'wait-scan' command to /proc/scsi/scsi.

scsi_wait_scan.ko is a bad interface for a variety of reasons:
- once you load it, it stays in memory doing nothing
- if you need to call it again, you first need to check for the module
and unload it first
- waiting for scsi scans shouldn't require module loading privleges in
any case

This creates a simpler interface that doesn't require as much
scaffolding in userspace.

Signed-off-by: Bill Nottingham <[email protected]>
---
drivers/scsi/scsi_proc.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/scsi_proc.c b/drivers/scsi/scsi_proc.c
index 82f7b2d..c9f9e0c 100644
--- a/drivers/scsi/scsi_proc.c
+++ b/drivers/scsi/scsi_proc.c
@@ -370,6 +370,8 @@ static ssize_t proc_scsi_write(struct file *file, const char __user *buf,
lun = simple_strtoul(p + 1, &p, 0);

err = scsi_remove_single_device(host, channel, id, lun);
+ } else if (!strncmp("scsi wait-scan", buffer, 14)) {
+ err = scsi_complete_async_scans();
}

/*
--
1.6.2


2009-03-25 19:01:26

by Bill Nottingham

[permalink] [raw]
Subject: [PATCH] Declare PIO_CMAP/GIO_CMAP as compatbile ioctls.

Otherwise, these don't work when called from 32-bit userspace on 64-bit kernels.
---
fs/compat_ioctl.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
index 5235c67..9ee71a0 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -1937,6 +1937,8 @@ ULONG_IOCTL(SET_BITMAP_FILE)
/* Big K */
COMPATIBLE_IOCTL(PIO_FONT)
COMPATIBLE_IOCTL(GIO_FONT)
+COMPATIBLE_IOCTL(PIO_CMAP)
+COMPATIBLE_IOCTL(GIO_CMAP)
ULONG_IOCTL(KDSIGACCEPT)
COMPATIBLE_IOCTL(KDGETKEYCODE)
COMPATIBLE_IOCTL(KDSETKEYCODE)
--
1.6.1.2

2009-03-25 19:01:53

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH] Add a 'wait-scan' command to /proc/scsi/scsi.

On Wed, Mar 25, 2009 at 03:00:46PM -0400, Bill Nottingham wrote:
> scsi_wait_scan.ko is a bad interface for a variety of reasons:
> - once you load it, it stays in memory doing nothing
> - if you need to call it again, you first need to check for the module
> and unload it first
> - waiting for scsi scans shouldn't require module loading privleges in
> any case
>
> This creates a simpler interface that doesn't require as much
> scaffolding in userspace.

/proc/scsi/scsi is deprecated, plese don't add more features to it.

2009-03-25 19:02:41

by Bill Nottingham

[permalink] [raw]
Subject: Re: [PATCH] Declare PIO_CMAP/GIO_CMAP as compatbile ioctls.

Bill Nottingham ([email protected]) said:
> Otherwise, these don't work when called from 32-bit userspace on 64-bit kernels.

Sorry about this last bit - git hiccup.

Bill

2009-03-25 19:04:50

by Bill Nottingham

[permalink] [raw]
Subject: Re: [PATCH] Add a 'wait-scan' command to /proc/scsi/scsi.

Christoph Hellwig ([email protected]) said:
> On Wed, Mar 25, 2009 at 03:00:46PM -0400, Bill Nottingham wrote:
> > scsi_wait_scan.ko is a bad interface for a variety of reasons:
> > - once you load it, it stays in memory doing nothing
> > - if you need to call it again, you first need to check for the module
> > and unload it first
> > - waiting for scsi scans shouldn't require module loading privleges in
> > any case
> >
> > This creates a simpler interface that doesn't require as much
> > scaffolding in userspace.
>
> /proc/scsi/scsi is deprecated, plese don't add more features to it.

Then where is a better place to put this, as scsi_wait_scan.ko is
a ridiculous interface for userspace?

Bill

2009-03-25 19:27:21

by Matthew Wilcox

[permalink] [raw]
Subject: Re: [PATCH] Add a 'wait-scan' command to /proc/scsi/scsi.

On Wed, Mar 25, 2009 at 03:03:21PM -0400, Bill Nottingham wrote:
> Then where is a better place to put this, as scsi_wait_scan.ko is
> a ridiculous interface for userspace?

It would be nice if people would comment on "ridiculous interface"s when
they're asked for feedback, instead of waiting more than two years.

I think you're misunderstanding how to use scsi_wait_scan. The idea was
that the bit of userspace that probes all the device drivers would do:

modprobe fusion.ko
modprobe aic79xx.ko
modprobe sym53c8xx.ko
modprobe scsi_wait_scan
rmmod scsi_wait_scan

et voila, you're done. It seems like you want random other bits of
userspace to wait for scsi scanning to be done, and that wasn't the
original intent.


I suppose we could add Yet Another Damn Sysfs File, say
/sys/bus/scsi/wait_scan

--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."

2009-03-25 19:48:22

by Bill Nottingham

[permalink] [raw]
Subject: Re: [PATCH] Add a 'wait-scan' command to /proc/scsi/scsi.

Matthew Wilcox ([email protected]) said:
> On Wed, Mar 25, 2009 at 03:03:21PM -0400, Bill Nottingham wrote:
> > Then where is a better place to put this, as scsi_wait_scan.ko is
> > a ridiculous interface for userspace?
>
> It would be nice if people would comment on "ridiculous interface"s when
> they're asked for feedback, instead of waiting more than two years.

Sure, but asking all people who might eventually have to use it
to always watch any possible interface addition isn't practical.

I would have hoped that the fact that the interface required loading
a module and immediately removing it by hand is suboptimal enough
that it wouldn't have gotten in in the first place.

> I think you're misunderstanding how to use scsi_wait_scan. The idea was
> that the bit of userspace that probes all the device drivers would do:
>
> modprobe fusion.ko
> modprobe aic79xx.ko
> modprobe sym53c8xx.ko
> modprobe scsi_wait_scan
> rmmod scsi_wait_scan
>
> et voila, you're done. It seems like you want random other bits of
> userspace to wait for scsi scanning to be done, and that wasn't the
> original intent.

Well, in the case I'm looking at, udev is what's loading the host
controllers, and there needs to be some sort of synchronization point
between that and LVM invocations, fsck, mount, etc. Since scans
aren't sent over as events for udev to catch, 'udevadm settle'
isn't enough. Removing, loading, and removing scsi_wait_scan works
here, but it just seems like a kludge.

I can trigger a load of scsi_wait_scan when hosts are registered
in udev, but that's still ugly, and sort of overkill.

Bill

2009-03-25 20:36:52

by Matthew Wilcox

[permalink] [raw]
Subject: Re: [PATCH] Add a 'wait-scan' command to /proc/scsi/scsi.

On Wed, Mar 25, 2009 at 03:47:56PM -0400, Bill Nottingham wrote:
> Matthew Wilcox ([email protected]) said:
> > On Wed, Mar 25, 2009 at 03:03:21PM -0400, Bill Nottingham wrote:
> > > Then where is a better place to put this, as scsi_wait_scan.ko is
> > > a ridiculous interface for userspace?
> >
> > It would be nice if people would comment on "ridiculous interface"s when
> > they're asked for feedback, instead of waiting more than two years.
>
> Sure, but asking all people who might eventually have to use it
> to always watch any possible interface addition isn't practical.

Right. I asked several people at Red Hat about the interface and I got a
"yeah, OK, whatever" kind of response. Clearly you need to educate your
colleagues to pass these kinds of interface questions along to you.

> I would have hoped that the fact that the interface required loading
> a module and immediately removing it by hand is suboptimal enough
> that it wouldn't have gotten in in the first place.

It seems pretty elegant to me, actually. There's no overhead after
you're done (unlike having a sysfs file, or even including a new ability
in a procfs file).

> > I think you're misunderstanding how to use scsi_wait_scan. The idea was
> > that the bit of userspace that probes all the device drivers would do:
> >
> > modprobe fusion.ko
> > modprobe aic79xx.ko
> > modprobe sym53c8xx.ko
> > modprobe scsi_wait_scan
> > rmmod scsi_wait_scan
> >
> > et voila, you're done. It seems like you want random other bits of
> > userspace to wait for scsi scanning to be done, and that wasn't the
> > original intent.
>
> Well, in the case I'm looking at, udev is what's loading the host
> controllers, and there needs to be some sort of synchronization point
> between that and LVM invocations, fsck, mount, etc. Since scans
> aren't sent over as events for udev to catch, 'udevadm settle'
> isn't enough.

So ... if we sent a udev event when the scan list was empty, you'd be OK?

> Removing, loading, and removing scsi_wait_scan works
> here, but it just seems like a kludge.

I don't quite understand why it was loaded, and not unloaded immediately.

> I can trigger a load of scsi_wait_scan when hosts are registered
> in udev, but that's still ugly, and sort of overkill.

That would rather miss the point, yes.

--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."

2009-03-26 14:47:42

by Bill Nottingham

[permalink] [raw]
Subject: Re: [PATCH] Add a 'wait-scan' command to /proc/scsi/scsi.

Matthew Wilcox ([email protected]) said:
> > Sure, but asking all people who might eventually have to use it
> > to always watch any possible interface addition isn't practical.
>
> Right. I asked several people at Red Hat about the interface and I got a
> "yeah, OK, whatever" kind of response. Clearly you need to educate your
> colleagues to pass these kinds of interface questions along to you.

Heh. Of course, I suspect many of them and I wouldn't see eye-to-eye
in any case.

> > Well, in the case I'm looking at, udev is what's loading the host
> > controllers, and there needs to be some sort of synchronization point
> > between that and LVM invocations, fsck, mount, etc. Since scans
> > aren't sent over as events for udev to catch, 'udevadm settle'
> > isn't enough.
>
> So ... if we sent a udev event when the scan list was empty, you'd be OK?

I'm CC'ing Peter, who has some more ideas - it would definitely be a good
start, but we'd probably at least need to know when the scan list started
being filled as well.

This also doesn't help with USB, where the scan isn't scheduled until some
indeterminate time after the host is registered, but given that USB is
always hotpluggable, you can never fully be sure there.

Ideally, once we get to the point where:

- MD
- DM/LVM/etc
- fsck
- mount

can all be event-driven on startup, we won't need this. But we're not
there yet.

> > Removing, loading, and removing scsi_wait_scan works
> > here, but it just seems like a kludge.
>
> I don't quite understand why it was loaded, and not unloaded immediately.

In the initramfs (as we use it) it is, but any time later in userspace it
seems prudent to make sure it's removed first. I toyed with the idea of
having module_init() return an error so that the module unregisters itself
after doing the wait-for-scans, but the error of course propagates back
to userspace, giving lots of ugly messages in the default config.
(You could do a modprobe rule that automatically removes it after
insertion.)

Bill

2009-03-26 17:25:46

by Kay Sievers

[permalink] [raw]
Subject: Re: [PATCH] Add a 'wait-scan' command to /proc/scsi/scsi.

On Thu, Mar 26, 2009 at 15:47, Bill Nottingham <[email protected]> wrote:
> Matthew Wilcox ([email protected]) said:

>> > Well, in the case I'm looking at, udev is what's loading the host
>> > controllers, and there needs to be some sort of synchronization point
>> > between that and LVM invocations, fsck, mount, etc. Since scans
>> > aren't sent over as events for udev to catch, 'udevadm settle'
>> > isn't enough.
>>
>> So ... if we sent a udev event when the scan list was empty, you'd be OK?
>
> I'm CC'ing Peter, who has some more ideas - it would definitely be a good
> start, but we'd probably at least need to know when the scan list started
> being filled as well.

I don't like to see any uevent send for issues like this. This would
just be a hack around badly designed system services, which should be
fixed themselves and not worked around in the kernel. This hack will
run useless events for system who don't need them.

Something like a poll()'able sysfs file, which other subsytems already
use, and which can be used to wake up a process that can blocks, might
work fine here.

Thanks,
Kay