[PATCH] Add MOUNT_STATUS ioctl to cdrom device

[PATCH] Add MOUNT_STATUS ioctl to cdrom device

[Samphan Raruenrom]

Hi,

This patch add a new ioctl MOUNT_STATUS to the 2.4 kernel's cdrom
device. It'll be used as an API to query the mount status of a
cdrom :-

CDROM_MOUNT_STATUS
Return :-
0 = not mount.
1 = mounted, but not in-use. It is ok to umount.
2 = busy. Umount will result in getting EBUSY.
<0 = error.

This same functionality can be done in user-space, like the way
'fuser' do, but doing so in kernel-space is simpler and faster.
The ioctl can (will) be used by automount daemon to query the
device to see if it is in use or not and lock/unlock the drive
as needed. This will make the cdrom "eject" button work as it
should be (as auto-umount button). Novice GUI users usually wonder
why they can't eject cds even when the discs are not in use.

An experimental implementation has been done and working on my machine.
I'm completely new to kernel hacking so please make comments about
this patch and whether it's justified or plain stupid to add another
ioctl for this feature cause I really have no idea.



diff -dur linux-2.4.22-pre10.orig/drivers/cdrom/cdrom.c linux-2.4.22-pre10/drivers/cdrom/cdrom.c
--- linux-2.4.22-pre10.orig/drivers/cdrom/cdrom.c	2002-11-29 06:53:12.000000000 +0700
+++ linux-2.4.22-pre10/drivers/cdrom/cdrom.c	2003-08-26 00:10:22.000000000 +0700
@@ -264,6 +264,8 @@
  #include <linux/sysctl.h>
  #include <linux/proc_fs.h>
  #include <linux/init.h>
+#include <linux/namespace.h>
+#include <linux/mount.h>

  #include <asm/fcntl.h>
  #include <asm/segment.h>
@@ -1622,6 +1624,26 @@
  		return cdo->lock_door(cdi, arg);
  		}

+	case CDROM_MOUNT_STATUS: {
+		struct super_block *sb = get_super(dev);
+		if (sb == NULL) return -EINVAL;
+		down_read(&current->namespace->sem);
+		struct vfsmount *mnt = NULL;
+		struct list_head *p;
+		list_for_each(p, &current->namespace->list) {
+			struct vfsmount *m = list_entry(p, struct vfsmount, mnt_list);
+			if (sb == m->mnt_sb) {
+				mnt = m; break;
+			}
+		}
+		up_read(&current->namespace->sem);		
+		drop_super(sb);		
+		int mstat = 0; /* 0 not mounted, 1 umount ok, 2 umount EBUSY */
+		if (mnt) mstat = 1 + (atomic_read(&mnt->mnt_count) > 1);
+		cdinfo(CD_DO_IOCTL, "mount status(%s) = %d\n", mnt->mnt_devname, mstat);
+		return mstat;
+		}
+	
  	case CDROM_DEBUG: {
  		if (!capable(CAP_SYS_ADMIN))
  			return -EACCES;
diff -dur linux-2.4.22-pre10.orig/include/linux/cdrom.h linux-2.4.22-pre10/include/linux/cdrom.h
--- linux-2.4.22-pre10.orig/include/linux/cdrom.h	2003-08-05 02:59:58.000000000 +0700
+++ linux-2.4.22-pre10/include/linux/cdrom.h	2003-08-13 21:41:27.000000000 +0700
@@ -127,6 +127,7 @@
  #define CDROM_LOCKDOOR		0x5329  /* lock or unlock door */
  #define CDROM_DEBUG		0x5330	/* Turn debug messages on/off */
  #define CDROM_GET_CAPABILITY	0x5331	/* get capabilities */
+#define CDROM_MOUNT_STATUS	0x5332	/* is it mounted? can umount? */

  /* Note that scsi/scsi_ioctl.h also uses 0x5382 - 0x5386.
   * Future CDROM ioctls should be kept below 0x537F


-- 
Samphan Raruenrom,
The Open Source Project,
National Electronics and Computer Technology Center,
National Science and Technology Development Agency,
Thailand.

Re: [PATCH] Add MOUNT_STATUS ioctl to cdrom device

[Christoph Hellwig]

On Tue, Aug 26, 2003 at 01:22:37AM +0700, Samphan Raruenrom wrote:
>   #include <linux/sysctl.h>
>   #include <linux/proc_fs.h>
>   #include <linux/init.h>
> +#include <linux/namespace.h>
> +#include <linux/mount.h>

your mailer is screwed.

> 
> +	case CDROM_MOUNT_STATUS: {
> +		struct super_block *sb = get_super(dev);
> +		if (sb == NULL) return -EINVAL;
> +		down_read(&current->namespace->sem);
> +		struct vfsmount *mnt = NULL;
> +		struct list_head *p;
> +		list_for_each(p, &current->namespace->list) {
> +			struct vfsmount *m = list_entry(p, struct vfsmount, mnt_list);
> +			if (sb == m->mnt_sb) {
> +				mnt = m; break;
> +			}
> +		}
> +		up_read(&current->namespace->sem);		
> +		drop_super(sb);		
> +		int mstat = 0; /* 0 not mounted, 1 umount ok, 2 umount EBUSY */
> +		if (mnt) mstat = 1 + (atomic_read(&mnt->mnt_count) > 1);
> +		cdinfo(CD_DO_IOCTL, "mount status(%s) = %d\n", mnt->mnt_devname, mstat);
> +		return mstat;

WTF?  This is not only a layering violation but also totally racy.

Rejected.

Re: [PATCH] Add MOUNT_STATUS ioctl to cdrom device

[viro]

On Tue, Aug 26, 2003 at 01:22:37AM +0700, Samphan Raruenrom wrote:
> Hi,
> 
> This patch add a new ioctl MOUNT_STATUS to the 2.4 kernel's cdrom
> device. It'll be used as an API to query the mount status of a
> cdrom :-
> 
> CDROM_MOUNT_STATUS
> Return :-
> 0 = not mount.
> 1 = mounted, but not in-use. It is ok to umount.
> 2 = busy. Umount will result in getting EBUSY.
> <0 = error.

Huh?   And what, pray tell, makes cdrom special?  Not to mention
the use of ioctl, the inherent raciness of the interface and the fact
that yes,
 
> This same functionality can be done in user-space,

... which should be the end of it.

> +	case CDROM_MOUNT_STATUS: {
> +		struct super_block *sb = get_super(dev);
> +		if (sb == NULL) return -EINVAL;
> +		down_read(&current->namespace->sem);
> +		struct vfsmount *mnt = NULL;
> +		struct list_head *p;
> +		list_for_each(p, &current->namespace->list) {
> +			struct vfsmount *m = list_entry(p, struct vfsmount, 
> mnt_list);
> +			if (sb == m->mnt_sb) {
> +				mnt = m; break;
> +			}
> +		}
> +		up_read(&current->namespace->sem);		

And what about other namespaces?

> +		drop_super(sb);		
> +		int mstat = 0; /* 0 not mounted, 1 umount ok, 2 umount EBUSY 
> */
> +		if (mnt) mstat = 1 + (atomic_read(&mnt->mnt_count) > 1);

Or the possibility that
	* mnt might've been freed by that point.
	* we might have the damn thing mounted in several places, some
busy, some not.
	* cdrom had been used not by mount.
	* cdrom had been mounted just as we had decided to tell that it's
not busy.

Re: [PATCH] Add MOUNT_STATUS ioctl to cdrom device

[Samphan Raruenrom]

Christoph Hellwig wrote:
>>+	case CDROM_MOUNT_STATUS: {
>>+		struct super_block *sb = get_super(dev);
>>+		if (sb == NULL) return -EINVAL;
>>+		down_read(&current->namespace->sem);
>>+		struct vfsmount *mnt = NULL;
>>+		struct list_head *p;
>>+		list_for_each(p, &current->namespace->list) {
>>+			struct vfsmount *m = list_entry(p, struct vfsmount, mnt_list);
>>+			if (sb == m->mnt_sb) {
>>+				mnt = m; break;
>>+			}
>>+		}
>>+		up_read(&current->namespace->sem);		
>>+		drop_super(sb);		
>>+		int mstat = 0; /* 0 not mounted, 1 umount ok, 2 umount EBUSY */
>>+		if (mnt) mstat = 1 + (atomic_read(&mnt->mnt_count) > 1);
>>+		cdinfo(CD_DO_IOCTL, "mount status(%s) = %d\n", mnt->mnt_devname, mstat);
>>+		return mstat;
> WTF?  This is not only a layering violation but also totally racy.

I'm sorry for issueing this layering violation. I read a guideline that
it's easier to submit a patch to add device driver ioctl than inventing
something new. It really doesn't belong here.
Could you guide me where else can I place this functionality?

(my random idea)
- fcntl(open("/dev/cdrom", F_MNTSTAT)
- umount2("/dev/cdrom", MS_TEST) // not actually perform
- new system call! e.g. mntstat(open("/dev/cdrom"))

-- 
Samphan Raruenrom,
The Open Source Project,
National Electronics and Computer Technology Center,
National Science and Technology Development Agency,
Thailand.

Re: [PATCH] Add MOUNT_STATUS ioctl to cdrom device

[Christoph Hellwig]

On Tue, Aug 26, 2003 at 01:50:43PM +0700, Samphan Raruenrom wrote:
> I'm sorry for issueing this layering violation. I read a guideline that
> it's easier to submit a patch to add device driver ioctl than inventing
> something new. It really doesn't belong here.

That guideline is wrong.  Adding a driver ioctl is slmost always wrong.

> Could you guide me where else can I place this functionality?
> 
> (my random idea)
> - fcntl(open("/dev/cdrom", F_MNTSTAT)
> - umount2("/dev/cdrom", MS_TEST) // not actually perform
> - new system call! e.g. mntstat(open("/dev/cdrom"))

In userspace.  Or you could tell me what you want to actually
archive - your call by itself doesn't make any sense.

Re: [PATCH] Add MOUNT_STATUS ioctl to cdrom device

[Samphan Raruenrom]

Christoph Hellwig wrote:
>>(my random idea)
>>- fcntl(open("/dev/cdrom", F_MNTSTAT)
>>- umount2("/dev/cdrom", MS_TEST) // not actually perform
>>- new system call! e.g. mntstat(open("/dev/cdrom"))
> In userspace.  Or you could tell me what you want to actually
> archive - your call by itself doesn't make any sense.

To get the same result in userspace, it means scanning /proc
like fuser, right?  Do you think it is ok to do that?  Because
the daemon (patched magicdev) poll the cdrom device very often
(every 2 sec.)
That's why I thought that faster is better (less load to
the system) and push the job to kernel-space.

This is what I really want to achive - "enable the eject button,
by locking the drive only when the device is really in use,
unlock otherwise".

I implement this as a modified version of GNOME magicdev.
http://bugzilla.gnome.org/show_bug.cgi?id=119892

The only visible feature of this new magicdev is that now
GNOME users can eject there CDs (the discs' icon will
disappear). The eject button now act as 'umount' command.

One new requirement from this new magicdev is the question
"will umount failed?". I have no preference on any way to
implement it. Should there be the right way to do it, I'll
do so. I can think of many way to implement it (including
adding a new lazy-lock mode to cdrom device) but since
I have no kernel hacking experience, I need everyone
advices. Novice users need this 'eject' button after all.

-- 
Samphan Raruenrom,
The Open Source Project,
National Electronics and Computer Technology Center,
National Science and Technology Development Agency,
Thailand.

Re: [PATCH] Add MOUNT_STATUS ioctl to cdrom device

[Christoph Hellwig]

On Tue, Aug 26, 2003 at 04:09:54PM +0700, Samphan Raruenrom wrote:
> The only visible feature of this new magicdev is that now
> GNOME users can eject there CDs (the discs' icon will
> disappear). The eject button now act as 'umount' command.
> 
> One new requirement from this new magicdev is the question
> "will umount failed?". I have no preference on any way to
> implement it. Should there be the right way to do it, I'll
> do so. I can think of many way to implement it (including
> adding a new lazy-lock mode to cdrom device) but since
> I have no kernel hacking experience, I need everyone
> advices. Novice users need this 'eject' button after all.

This doesn't make sense at all.  Just try the unmount and
tell the user if it failed - you can't say whether it will
fail before trying.

Re: [PATCH] Add MOUNT_STATUS ioctl to cdrom device

[Jens Axboe]

On Tue, Aug 26 2003, Christoph Hellwig wrote:
> On Tue, Aug 26, 2003 at 04:09:54PM +0700, Samphan Raruenrom wrote:
> > The only visible feature of this new magicdev is that now
> > GNOME users can eject there CDs (the discs' icon will
> > disappear). The eject button now act as 'umount' command.
> > 
> > One new requirement from this new magicdev is the question
> > "will umount failed?". I have no preference on any way to
> > implement it. Should there be the right way to do it, I'll
> > do so. I can think of many way to implement it (including
> > adding a new lazy-lock mode to cdrom device) but since
> > I have no kernel hacking experience, I need everyone
> > advices. Novice users need this 'eject' button after all.
> 
> This doesn't make sense at all.  Just try the unmount and
> tell the user if it failed - you can't say whether it will
> fail before trying.

Exactly. You poll media events from the drive, and upon an eject request
you try and umount it. If it suceeds, you eject the tray. It's pretty
simple, really.

This mount status patch is seriously broken and misguided.

-- 
Jens Axboe

Re: [Rdi1] Re: [PATCH] Add MOUNT_STATUS ioctl to cdrom device

[Samphan Raruenrom]

Jens Axboe wrote:
> On Tue, Aug 26 2003, Christoph Hellwig wrote:
>>On Tue, Aug 26, 2003 at 04:09:54PM +0700, Samphan Raruenrom wrote:
>>>The only visible feature of this new magicdev is that now
>>>GNOME users can eject there CDs (the discs' icon will
>>>disappear). The eject button now act as 'umount' command.
>>>One new requirement from this new magicdev is the question
>>>"will umount failed?". I have no preference on any way to
>>>implement it. Should there be the right way to do it, I'll
>>>do so. I can think of many way to implement it (including
>>>adding a new lazy-lock mode to cdrom device) but since
>>>I have no kernel hacking experience, I need everyone
>>>advices. Novice users need this 'eject' button after all.
>>This doesn't make sense at all.  Just try the unmount and
>>tell the user if it failed - you can't say whether it will
>>fail before trying.

Yes, you can! Reading the code, if vfsmount.mnt_count > 1 then
umount on that device will fail. Similar function is used
by autofs, i.e. may_umount() in fs/namespace.c. I know the patch
is not beautiful but it work.
Hmm. You seem to advice me to detect the 'eject' button then issue
a umount? I don't know how and I guess it is impossible. But maybe
I'm wrong.

> Exactly. You poll media events from the drive, and upon an eject request
> you try and umount it. If it suceeds, you eject the tray. 

No, it seems impossible to sense the eject request (right?). This
is what I really did with the patched kernel and patched magicdev.
When, for example:-

- user insert CD
- magicdev sense the CD, mount /dev/cdrom -> eject is ok
- user play an mp3 file in the disc
- magicdev found MOUNT_STATUS=BUSY, lock the drive -> eject disabled
- user stop using any file on the disc
- magicdev found MOUNT_STATUS=MOUNTED, unlock the drive -> eject is ok
- user push the eject button, the disc pop out because it's not locked.
- magicdev sense empty drive, umount /dev/cdrom


-- 
Samphan Raruenrom,
The Open Source Project,
National Electronics and Computer Technology Center,
National Science and Technology Development Agency,
Thailand.

Re: [Rdi1] Re: [PATCH] Add MOUNT_STATUS ioctl to cdrom device

[Jens Axboe]

On Tue, Aug 26 2003, Samphan Raruenrom wrote:
> >Exactly. You poll media events from the drive, and upon an eject request
> >you try and umount it. If it suceeds, you eject the tray. 
> 
> No, it seems impossible to sense the eject request (right?). This

No it isn't, in fact there are several ways to do it. Just by searching
this list you should be able to find them.

> is what I really did with the patched kernel and patched magicdev.

magicdev is a piece of crap.

> When, for example:-
> 
> - user insert CD
> - magicdev sense the CD, mount /dev/cdrom -> eject is ok
> - user play an mp3 file in the disc
> - magicdev found MOUNT_STATUS=BUSY, lock the drive -> eject disabled
> - user stop using any file on the disc
> - magicdev found MOUNT_STATUS=MOUNTED, unlock the drive -> eject is ok
> - user push the eject button, the disc pop out because it's not locked.
> - magicdev sense empty drive, umount /dev/cdrom

That's not the way to do it. For drives that support media status event
reporting you don't lock the drive, you just intercept the eject
request. And at that time you check if you can umount /cdrom (or
whatever) - if that suceeds, you send an eject request to the drive.
This will work with any drive made in the last few years. You can also
check out MS media status notification, I think you can even find that
document on microsoft.com.

I think you need to spend a little more time thinking/researching this
problem. At least it really looks like you are going about it all wrong.

-- 
Jens Axboe

Re: [Rdi1] Re: [PATCH] Add MOUNT_STATUS ioctl to cdrom device

[Samphan Raruenrom]

Jens Axboe wrote:
> On Tue, Aug 26 2003, Samphan Raruenrom wrote:
>>>Exactly. You poll media events from the drive, and upon an eject request
>>>you try and umount it. If it suceeds, you eject the tray. 
>>No, it seems impossible to sense the eject request (right?). This
> No it isn't, in fact there are several ways to do it. Just by searching
> this list you should be able to find them.

YES!! http://www.ussg.iu.edu/hypermail/linux/kernel/0202.0/att-0603/01-cd_poll.c
get_media_event() = 1 -> eject
Thanks :-)  I think you can't imagine how happy I am now. Thank you again.

>>is what I really did with the patched kernel and patched magicdev.
> magicdev is a piece of crap.

Why? I read all its code. Because of 2 sec. polling?

> I think you need to spend a little more time thinking/researching this
> problem. At least it really looks like you are going about it all wrong.

You've just put me on the right track. Thank you very much. I really
appreciate your and every others insightful comments.

Re: [Rdi1] Re: [PATCH] Add MOUNT_STATUS ioctl to cdrom device

[Christoph Hellwig]

On Tue, Aug 26, 2003 at 06:30:10PM +0700, Samphan Raruenrom wrote:
> >>This doesn't make sense at all.  Just try the unmount and
> >>tell the user if it failed - you can't say whether it will
> >>fail before trying.
> 
> Yes, you can! Reading the code, if vfsmount.mnt_count > 1 then
> umount on that device will fail.

if you are doing the unmount currently and nothing changes because
you hold the nessecary locks, yes.  But as soon as you drop the locks
this is void.  There's no way to find out whether you can unmount
a filesystem except trying it.

> Hmm. You seem to advice me to detect the 'eject' button then issue
> a umount? I don't know how and I guess it is impossible. But maybe
> I'm wrong.

See Jens' post.

Re: [Rdi1] Re: [PATCH] Add MOUNT_STATUS ioctl to cdrom device

[Jens Axboe]

On Tue, Aug 26 2003, Samphan Raruenrom wrote:
> Jens Axboe wrote:
> >On Tue, Aug 26 2003, Samphan Raruenrom wrote:
> >>>Exactly. You poll media events from the drive, and upon an eject request
> >>>you try and umount it. If it suceeds, you eject the tray. 
> >>No, it seems impossible to sense the eject request (right?). This
> >No it isn't, in fact there are several ways to do it. Just by searching
> >this list you should be able to find them.
> 
> YES!! 
> http://www.ussg.iu.edu/hypermail/linux/kernel/0202.0/att-0603/01-cd_poll.c
> get_media_event() = 1 -> eject
> Thanks :-)  I think you can't imagine how happy I am now. Thank you again.

I'm surprised you didn't find these things up front.

> >>is what I really did with the patched kernel and patched magicdev.
> >magicdev is a piece of crap.
> 
> Why? I read all its code. Because of 2 sec. polling?

Because it relies on unreliable mechanisms instead of using the proper
support. That makes it 100% crap in my eyes, unusable.

> >I think you need to spend a little more time thinking/researching this
> >problem. At least it really looks like you are going about it all wrong.
> 
> You've just put me on the right track. Thank you very much. I really
> appreciate your and every others insightful comments.

No problem.

-- 
Jens Axboe

Re: [Rdi1] Re: [PATCH] Add MOUNT_STATUS ioctl to cdrom device

[Samphan Raruenrom]

Christoph Hellwig wrote:
 > On Tue, Aug 26, 2003 at 06:30:10PM +0700, Samphan Raruenrom wrote:
 >>>>This doesn't make sense at all.  Just try the unmount and
 >>>>tell the user if it failed - you can't say whether it will
 >>>>fail before trying.
 >>Yes, you can! Reading the code, if vfsmount.mnt_count > 1 then
 >>umount on that device will fail.
 > if you are doing the unmount currently and nothing changes because
 > you hold the nessecary locks, yes.  But as soon as you drop the locks
 > this is void.  There's no way to find out whether you can unmount
 > a filesystem except trying it.

_^_ I understand it now. Thank you for your patience in explaining this
to me. I learn quite a lot of things in <4 hours! Thank you everyone.
I'll try to make a good use of what I learn here, e.g. to write another
GNOME automounter that use the new "event status notification" and also
let users eject their CDs -
"without polling" - possible?
It must be real cool to write such daemon (polling make any hacker feel guilty).

-- 
Samphan Raruenrom,
The Open Source Project,
National Electronics and Computer Technology Center,
National Science and Technology Development Agency,
Thailand.

Re: [Rdi1] Re: [PATCH] Add MOUNT_STATUS ioctl to cdrom device

[Jens Axboe]

On Tue, Aug 26 2003, Samphan Raruenrom wrote:
> GNOME automounter that use the new "event status notification" and also
> let users eject their CDs -
> "without polling" - possible?
> It must be real cool to write such daemon (polling make any hacker feel 
> guilty).

I don't want to burst your bubble, but you still have to poll when using
event status notification. You didn't note that the name of my program
was cd_poll.c? ;-)

But the poll is much quicker and more reliable.

-- 
Jens Axboe