PATCH: fix-readonly-policy-use-and-floppy-ro-rw-status

PATCH: fix-readonly-policy-use-and-floppy-ro-rw-status

[Jon Masters]

[PATCH]: This modifies the gendisk and hd_struct structs to replace "policy"
with "readonly" (as that's the only use for this field). It also introduces a
new function disk_read_only, which behaves like the corresponding device
functions do. I've also replaced direct usage of the old policy fields with
calls to the appropriate function.

Signed-off-by: Jon Masters <jcm@jonmasters.org>

diff -urN linux-2.6.14/drivers/block/floppy.c linux-2.6.14_new/drivers/block/floppy.c
--- linux-2.6.14/drivers/block/floppy.c	2005-10-28 01:02:08.000000000 +0100
+++ linux-2.6.14_new/drivers/block/floppy.c	2005-11-05 15:30:05.000000000 +0000
@@ -3714,6 +3714,13 @@
 		USETF(FD_VERIFY);
 	}
 
+	/* set underlying gendisk readonly state to reflect real ro/rw status */
+	if (UTESTF(FD_DISK_WRITABLE)) {
+		set_device_ro(inode->i_bdev, 0);
+	} else {
+		set_device_ro(inode->i_bdev, 1);
+	}
+	
 	if (UDRS->fd_ref == -1 || (UDRS->fd_ref && (filp->f_flags & O_EXCL)))
 		goto out2;
 
diff -urN linux-2.6.14/drivers/block/genhd.c linux-2.6.14_new/drivers/block/genhd.c
--- linux-2.6.14/drivers/block/genhd.c	2005-10-28 01:02:08.000000000 +0100
+++ linux-2.6.14_new/drivers/block/genhd.c	2005-11-05 16:17:18.000000000 +0000
@@ -655,22 +655,22 @@
 
 EXPORT_SYMBOL(put_disk);
 
-void set_device_ro(struct block_device *bdev, int flag)
+void set_device_ro(struct block_device *bdev, int state)
 {
 	if (bdev->bd_contains != bdev)
-		bdev->bd_part->policy = flag;
+		bdev->bd_part->readonly = state;
 	else
-		bdev->bd_disk->policy = flag;
+		bdev->bd_disk->readonly = state;
 }
 
 EXPORT_SYMBOL(set_device_ro);
 
-void set_disk_ro(struct gendisk *disk, int flag)
+void set_disk_ro(struct gendisk *disk, int state)
 {
 	int i;
-	disk->policy = flag;
+	disk->readonly = state;
 	for (i = 0; i < disk->minors - 1; i++)
-		if (disk->part[i]) disk->part[i]->policy = flag;
+		if (disk->part[i]) disk->part[i]->readonly = state;
 }
 
 EXPORT_SYMBOL(set_disk_ro);
@@ -680,13 +680,23 @@
 	if (!bdev)
 		return 0;
 	else if (bdev->bd_contains != bdev)
-		return bdev->bd_part->policy;
+		return bdev->bd_part->readonly;
 	else
-		return bdev->bd_disk->policy;
+		return bdev->bd_disk->readonly;
 }
 
 EXPORT_SYMBOL(bdev_read_only);
 
+int disk_read_only(struct gendisk *disk)
+{
+	if (!disk)
+		return 0;
+
+	return (disk->readonly);
+}
+
+EXPORT_SYMBOL(disk_read_only);
+
 int invalidate_partition(struct gendisk *disk, int index)
 {
 	int res = 0;
diff -urN linux-2.6.14/drivers/ide/ide-cd.c linux-2.6.14_new/drivers/ide/ide-cd.c
--- linux-2.6.14/drivers/ide/ide-cd.c	2005-10-28 01:02:08.000000000 +0100
+++ linux-2.6.14_new/drivers/ide/ide-cd.c	2005-11-05 17:27:58.000000000 +0000
@@ -313,7 +313,7 @@
 #include <linux/cdrom.h>
 #include <linux/ide.h>
 #include <linux/completion.h>
-
+ 
 #include <scsi/scsi.h>	/* For SCSI -> ATAPI command conversion */
 
 #include <asm/irq.h>
@@ -1873,7 +1873,8 @@
 	/*
 	 * disk has become write protected
 	 */
-	if (g->policy) {
+
+	if (disk_read_only(g)) {
 		cdrom_end_request(drive, 0);
 		return ide_stopped;
 	}
diff -urN linux-2.6.14/drivers/md/dm-ioctl.c linux-2.6.14_new/drivers/md/dm-ioctl.c
--- linux-2.6.14/drivers/md/dm-ioctl.c	2005-10-28 01:02:08.000000000 +0100
+++ linux-2.6.14_new/drivers/md/dm-ioctl.c	2005-11-05 17:27:37.000000000 +0000
@@ -538,7 +538,7 @@
 	} else
 		param->open_count = -1;
 
-	if (disk->policy)
+	if (disk_read_only(disk))
 		param->flags |= DM_READONLY_FLAG;
 
 	param->event_nr = dm_get_event_nr(md);
diff -urN linux-2.6.14/include/linux/genhd.h linux-2.6.14_new/include/linux/genhd.h
--- linux-2.6.14/include/linux/genhd.h	2005-10-28 01:02:08.000000000 +0100
+++ linux-2.6.14_new/include/linux/genhd.h	2005-11-05 16:19:48.000000000 +0000
@@ -79,7 +79,7 @@
 	sector_t nr_sects;
 	struct kobject kobj;
 	unsigned reads, read_sectors, writes, write_sectors;
-	int policy, partno;
+	int readonly, partno;
 };
 
 #define GENHD_FL_REMOVABLE			1
@@ -116,7 +116,7 @@
 	struct kobject kobj;
 
 	struct timer_rand_state *random;
-	int policy;
+	int readonly;			/* needed for underlying media */
 
 	atomic_t sync_io;		/* RAID */
 	unsigned long stamp, stamp_idle;
@@ -230,8 +230,9 @@
 extern void unlink_gendisk(struct gendisk *gp);
 extern struct gendisk *get_gendisk(dev_t dev, int *part);
 
-extern void set_device_ro(struct block_device *bdev, int flag);
-extern void set_disk_ro(struct gendisk *disk, int flag);
+extern void set_device_ro(struct block_device *bdev, int state);
+extern void set_disk_ro(struct gendisk *disk, int state);
+extern int disk_read_only(struct gendisk *disk);
 
 /* drivers/char/random.c */
 extern void add_disk_randomness(struct gendisk *disk);

Re: PATCH: fix-readonly-policy-use-and-floppy-ro-rw-status

[Andrew Morton]

Jon Masters <jonathan@jonmasters.org> wrote:
>
> [PATCH]: This modifies the gendisk and hd_struct structs to replace "policy"
>  with "readonly" (as that's the only use for this field). It also introduces a
>  new function disk_read_only, which behaves like the corresponding device
>  functions do. I've also replaced direct usage of the old policy fields with
>  calls to the appropriate function.

These are separate things and should be done in separate patches, please.

Because, for exmaple, we may decide to revert the floppy change only. 
Because, as I said off-list, being able to do `remount,rw' of a floppy after
having flipped its switch is useful.

Re: PATCH: fix-readonly-policy-use-and-floppy-ro-rw-status

[Jon Masters]

On Sat, Nov 05, 2005 at 10:33:58AM -0800, Andrew Morton wrote:

> Jon Masters <jonathan@jonmasters.org> wrote:
> >
> > [PATCH]: This modifies the gendisk and hd_struct structs to replace "policy"
> >  with "readonly" (as that's the only use for this field). It also introduces a
> >  new function disk_read_only, which behaves like the corresponding device
> >  functions do. I've also replaced direct usage of the old policy fields with
> >  calls to the appropriate function.
> 
> These are separate things and should be done in separate patches, please.

I'm following up with another patch that just has the first bit (floppy)
removed.

> Because, for exmaple, we may decide to revert the floppy change only. 
> Because, as I said off-list, being able to do `remount,rw' of a floppy after
> having flipped its switch is useful.

And as I said, the situation as it stands leads to potential data
corruption but I agree with you - we need a VFS callback to handle
readwrite/readonly change on remount I think. Comments?

Jon.

Re: PATCH: fix-readonly-policy-use-and-floppy-ro-rw-status

[Jeff Garzik]

On Sat, Nov 05, 2005 at 06:27:28PM +0000, Jon Masters wrote:
> [PATCH]: This modifies the gendisk and hd_struct structs to replace "policy"
> with "readonly" (as that's the only use for this field). It also introduces a
> new function disk_read_only, which behaves like the corresponding device
> functions do. I've also replaced direct usage of the old policy fields with
> calls to the appropriate function.
> 
> Signed-off-by: Jon Masters <jcm@jonmasters.org>

Please fix your patch format per http://linux.yyz.us/patch-format.html

Notably:

- Using "[PATCH] " not "PATCH: " in subject line.

- Don't repeat "[PATCH]" in text body, this must be manually edited out.

- This is English, not dashish.  Remove the dashes from the
  one-line description found in the subject line.  These must be
  hand-edited out, too.

Re: PATCH: fix-readonly-policy-use-and-floppy-ro-rw-status

[Al Viro]

On Sat, Nov 05, 2005 at 10:33:58AM -0800, Andrew Morton wrote:
> Jon Masters <jonathan@jonmasters.org> wrote:
> >
> > [PATCH]: This modifies the gendisk and hd_struct structs to replace "policy"
> >  with "readonly" (as that's the only use for this field).

... for now.  IOW, NAK on that part - it's *not* a boolean and it's
intended to be used for e.g control of caching behaviour.  Stuff
like "mark it r/o for now without losing information about hw situation"
also should go there.

Re: PATCH: fix-readonly-policy-use-and-floppy-ro-rw-status

[Al Viro]

On Sat, Nov 05, 2005 at 06:40:28PM +0000, Jon Masters wrote:
> And as I said, the situation as it stands leads to potential data
> corruption but I agree with you - we need a VFS callback to handle
> readwrite/readonly change on remount I think. Comments?

It's not that simple.  Filesystem side of ro/rw transitions is
messy as hell and no, "VFS callback" won't be enough.

Re: PATCH: fix-readonly-policy-use-and-floppy-ro-rw-status

[Jon Masters]

On 11/5/05, Al Viro <viro@ftp.linux.org.uk> wrote:

> On Sat, Nov 05, 2005 at 10:33:58AM -0800, Andrew Morton wrote:
> > Jon Masters <jonathan@jonmasters.org> wrote:
> > >
> > > [PATCH]: This modifies the gendisk and hd_struct structs to replace "policy"
> > >  with "readonly" (as that's the only use for this field).

> ... for now.  IOW, NAK on that part - it's *not* a boolean and it's
> intended to be used for e.g control of caching behaviour.  Stuff
> like "mark it r/o for now without losing information about hw situation"
> also should go there.

But it's not being used for that. The only *uses* I've seen of it
suggest that people are assuming it is a readonly marker.

If you *want* it to be used for such purposes, would you rather I sent
another patch that actually introduced read/write flags to make this
more obvious (if we must keep the existing field around)?

Jon.

Re: PATCH: fix-readonly-policy-use-and-floppy-ro-rw-status

[Jon Masters]

On 11/5/05, Al Viro <viro@ftp.linux.org.uk> wrote:
> On Sat, Nov 05, 2005 at 06:40:28PM +0000, Jon Masters wrote:
> > And as I said, the situation as it stands leads to potential data
> > corruption but I agree with you - we need a VFS callback to handle
> > readwrite/readonly change on remount I think. Comments?

> It's not that simple.  Filesystem side of ro/rw transitions is
> messy as hell

Agreed.

> "VFS callback" won't be enough.

Although strangely enough other similar stuff in the remount path
works just fine. I can already request that a filesystem gets
remounted read-only - what's so wrong with forcing that behaviour when
I ask for an impossible combination?

Jon.

Re: PATCH: fix-readonly-policy-use-and-floppy-ro-rw-status

[Al Viro]

On Sat, Nov 05, 2005 at 06:51:39PM +0000, Jon Masters wrote:
> On 11/5/05, Al Viro <viro@ftp.linux.org.uk> wrote:
> > On Sat, Nov 05, 2005 at 06:40:28PM +0000, Jon Masters wrote:
> > > And as I said, the situation as it stands leads to potential data
> > > corruption but I agree with you - we need a VFS callback to handle
> > > readwrite/readonly change on remount I think. Comments?
> 
> > It's not that simple.  Filesystem side of ro/rw transitions is
> > messy as hell
> 
> Agreed.
> 
> > "VFS callback" won't be enough.
> 
> Although strangely enough other similar stuff in the remount path
> works just fine. I can already request that a filesystem gets
> remounted read-only - what's so wrong with forcing that behaviour when
> I ask for an impossible combination?

->remount_fs() certainly can refuse to go r/w - you don't need anything
new for that, just don't leave MS_RDONLY in *flags.  The real trouble
starts when fs wants to go r/o on its own, e.g. when it sees an error
bad enough to warrant that.  And that, BTW, is very likely to require
more than just one bit in ->policy - we want all IO on that device
to fail until after we actually close it during umount.  As it is,
we can get anything, including block allocations (e.g. if we have
a dirty mapping and it gets written to disk).  OTOH, we don't want
it to be the same thing as genuine hardware readonly - when buggered
fs is umounted, we are very likely to do fsck on it, after all.

Re: PATCH: fix-readonly-policy-use-and-floppy-ro-rw-status

[Jon Masters]

On 11/5/05, Al Viro <viro@ftp.linux.org.uk> wrote:

> ->remount_fs() certainly can refuse to go r/w - you don't need anything
> new for that, just don't leave MS_RDONLY in *flags.

Sure.

> The real trouble starts when fs wants to go r/o on its own

Although the situation here is really the other way around -
filesystem is mounted readonly (at least in this case with floppies -
but there are probably others too) and wants to go rw. When asked, we
need to check that the media is able to do handle that transition.
Currently I've bodged floppy.c to store this in policy/readonly (which
is what got me looking at genhd last week) but really there should be
a better way - one where we do this once we get to do_remount_sb or
whatever. This seems to be missing functionality.

> e.g. when it sees an error bad enough to warrant that.  And that, BTW, is very
> likely to require more than just one bit in ->policy - we want all IO on that device
> to fail until after we actually close it during umount.  As it is, we can get anything,
> including block allocations (e.g. if we have a dirty mapping and it gets written to disk).

Ok. So what you're saying is that this is more complex than I'd
implied and you're right :-) But aside from that, you're the expert
and I'm willing to go be a patch monkey, so just tell me what you
think is worth trying...

Jon.

Re: PATCH: fix-readonly-policy-use-and-floppy-ro-rw-status

[Jon Masters]

On 11/5/05, Jeff Garzik <jgarzik@pobox.com> wrote:

> Please fix your patch format per http://linux.yyz.us/patch-format.html

Done.

> - Using "[PATCH] " not "PATCH: " in subject line.

That was already the case. I repeated the word PATCH in the body too however.

> - Don't repeat "[PATCH]" in text body, this must be manually edited out.

Ok. Fair enough.

> - This is English, not dashish.  Remove the dashes from the
>   one-line description found in the subject line.  These must be
>   hand-edited out, too.

I read that as Danish the first time :-) But then realised this was
because I'd based it off something I'd sent to Andrew last week. Fair
comment.

Jon.