Re: [PATCH] cleanup for fixing get_super() races
Alexander Viro wrote: > > On Sat, 28 Apr 2001, Martin Dalecki wrote: > > > I think in the context you are inventig the proposed function, > > the drivers has allways an inode at hand. And contrary to what Linus > > Read the patch. Almost all cases are of the "loop over partitions of foo" > kind. > > > says, drivers not just know about the devices they handle, they > > know about the data they should get - at least in the context > > of block devices. And then you could as well pass the inode, which > > is already containing a refference to the corresponding sb and > > save the whole get_super linear array lookup 8-). I think > > No, you don't. Moreover, inode of device (even if you had it) _doesn't_ > contain a reference to sb of filesystem mounted from that device. Ohhh sorry right, I just did forget to have an checking look at the code before actual rammbling... It must had been some reminiscent from some other expermient with the kernel code I did recently that confused me. Sorry again... > > the less kdev_t the better! It's overused already anyway, like > > for example in the whole SCSI code, where the functions in reality only > > want to pass the minor number to differentiate they behaviour... This however I still hold up... > > If you are gogin to flag the behaviour of the function, > > then please use a bitpattern of well definded flags as a parameter, > > in a similiar way like it's done for example in many GUI libraries > > (GTK, Motif and so on). This would make it far more readabel. > > /me looks at From: > OK, Albert, what have you done with real Martin? > > OK, whoever you are - no, "expandable" interfaces of that sort are > rotten idea. What we really need is to replace sync_dev with fsync_dev - > it _is_ correct in such context. That's it - 1 bit of information, no > bitmaps needed. > > /me is still boggled by the idea of somebody refereing to GTK as an > example of style... Ehm, only for the waythe flags get passed, not the rest of it. You know if I see some parameter, taking possible values 0, 1, 2 then I mostly think that there should be some concrete names given to them :-). - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] cleanup for fixing get_super() races
Alexander Viro wrote: On Sat, 28 Apr 2001, Martin Dalecki wrote: I think in the context you are inventig the proposed function, the drivers has allways an inode at hand. And contrary to what Linus Read the patch. Almost all cases are of the loop over partitions of foo kind. says, drivers not just know about the devices they handle, they know about the data they should get - at least in the context of block devices. And then you could as well pass the inode, which is already containing a refference to the corresponding sb and save the whole get_super linear array lookup 8-). I think No, you don't. Moreover, inode of device (even if you had it) _doesn't_ contain a reference to sb of filesystem mounted from that device. Ohhh sorry right, I just did forget to have an checking look at the code before actual rammbling... It must had been some reminiscent from some other expermient with the kernel code I did recently that confused me. Sorry again... the less kdev_t the better! It's overused already anyway, like for example in the whole SCSI code, where the functions in reality only want to pass the minor number to differentiate they behaviour... This however I still hold up... If you are gogin to flag the behaviour of the function, then please use a bitpattern of well definded flags as a parameter, in a similiar way like it's done for example in many GUI libraries (GTK, Motif and so on). This would make it far more readabel. /me looks at From: OK, Albert, what have you done with real Martin? OK, whoever you are - no, expandable interfaces of that sort are rotten idea. What we really need is to replace sync_dev with fsync_dev - it _is_ correct in such context. That's it - 1 bit of information, no bitmaps needed. /me is still boggled by the idea of somebody refereing to GTK as an example of style... Ehm, only for the waythe flags get passed, not the rest of it. You know if I see some parameter, taking possible values 0, 1, 2 then I mostly think that there should be some concrete names given to them :-). - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] cleanup for fixing get_super() races
On Sat, 28 Apr 2001, Martin Dalecki wrote: > I think in the context you are inventig the proposed function, > the drivers has allways an inode at hand. And contrary to what Linus Read the patch. Almost all cases are of the "loop over partitions of foo" kind. > says, drivers not just know about the devices they handle, they > know about the data they should get - at least in the context > of block devices. And then you could as well pass the inode, which > is already containing a refference to the corresponding sb and > save the whole get_super linear array lookup 8-). I think No, you don't. Moreover, inode of device (even if you had it) _doesn't_ contain a reference to sb of filesystem mounted from that device. > the less kdev_t the better! It's overused already anyway, like > for example in the whole SCSI code, where the functions in reality only > want to pass the minor number to differentiate they behaviour... > > If you are gogin to flag the behaviour of the function, > then please use a bitpattern of well definded flags as a parameter, > in a similiar way like it's done for example in many GUI libraries > (GTK, Motif and so on). This would make it far more readabel. /me looks at From: OK, Albert, what have you done with real Martin? OK, whoever you are - no, "expandable" interfaces of that sort are rotten idea. What we really need is to replace sync_dev with fsync_dev - it _is_ correct in such context. That's it - 1 bit of information, no bitmaps needed. /me is still boggled by the idea of somebody refereing to GTK as an example of style... - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] cleanup for fixing get_super() races
Alexander Viro wrote: > > On Fri, 27 Apr 2001, Alexander Viro wrote: > > > Fine with me. Actually in _all_ cases execept cdrom.c it's preceded by > > either sync_dev() or fsync_dev(). What do you think about pulling that > > into the same function? Actually, that's what I've done in namespace > > patch (name being invalidate_dev(), BTW ;-) The only problem I see > > here is the argument telling whether we want sync or fsync (or nothing). > > OTOH, I seriously suspect that we ought replace all sync_dev() cases > > with fsync_dev() anyway... Your opinion? > > Al > > PS: last time I've separated that part of patch was a couple months > ago. See if something similar to the variant below would be OK with > you (I'll rediff it): I think in the context you are inventig the proposed function, the drivers has allways an inode at hand. And contrary to what Linus says, drivers not just know about the devices they handle, they know about the data they should get - at least in the context of block devices. And then you could as well pass the inode, which is already containing a refference to the corresponding sb and save the whole get_super linear array lookup 8-). I think the less kdev_t the better! It's overused already anyway, like for example in the whole SCSI code, where the functions in reality only want to pass the minor number to differentiate they behaviour... If you are gogin to flag the behaviour of the function, then please use a bitpattern of well definded flags as a parameter, in a similiar way like it's done for example in many GUI libraries (GTK, Motif and so on). This would make it far more readabel. -- just my two euro-cent's... - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] cleanup for fixing get_super() races
Alexander Viro wrote: On Fri, 27 Apr 2001, Alexander Viro wrote: Fine with me. Actually in _all_ cases execept cdrom.c it's preceded by either sync_dev() or fsync_dev(). What do you think about pulling that into the same function? Actually, that's what I've done in namespace patch (name being invalidate_dev(), BTW ;-) The only problem I see here is the argument telling whether we want sync or fsync (or nothing). OTOH, I seriously suspect that we ought replace all sync_dev() cases with fsync_dev() anyway... Your opinion? Al PS: last time I've separated that part of patch was a couple months ago. See if something similar to the variant below would be OK with you (I'll rediff it): I think in the context you are inventig the proposed function, the drivers has allways an inode at hand. And contrary to what Linus says, drivers not just know about the devices they handle, they know about the data they should get - at least in the context of block devices. And then you could as well pass the inode, which is already containing a refference to the corresponding sb and save the whole get_super linear array lookup 8-). I think the less kdev_t the better! It's overused already anyway, like for example in the whole SCSI code, where the functions in reality only want to pass the minor number to differentiate they behaviour... If you are gogin to flag the behaviour of the function, then please use a bitpattern of well definded flags as a parameter, in a similiar way like it's done for example in many GUI libraries (GTK, Motif and so on). This would make it far more readabel. -- just my two euro-cent's... - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] cleanup for fixing get_super() races
On Sat, 28 Apr 2001, Martin Dalecki wrote: I think in the context you are inventig the proposed function, the drivers has allways an inode at hand. And contrary to what Linus Read the patch. Almost all cases are of the loop over partitions of foo kind. says, drivers not just know about the devices they handle, they know about the data they should get - at least in the context of block devices. And then you could as well pass the inode, which is already containing a refference to the corresponding sb and save the whole get_super linear array lookup 8-). I think No, you don't. Moreover, inode of device (even if you had it) _doesn't_ contain a reference to sb of filesystem mounted from that device. the less kdev_t the better! It's overused already anyway, like for example in the whole SCSI code, where the functions in reality only want to pass the minor number to differentiate they behaviour... If you are gogin to flag the behaviour of the function, then please use a bitpattern of well definded flags as a parameter, in a similiar way like it's done for example in many GUI libraries (GTK, Motif and so on). This would make it far more readabel. /me looks at From: OK, Albert, what have you done with real Martin? OK, whoever you are - no, expandable interfaces of that sort are rotten idea. What we really need is to replace sync_dev with fsync_dev - it _is_ correct in such context. That's it - 1 bit of information, no bitmaps needed. /me is still boggled by the idea of somebody refereing to GTK as an example of style... - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] cleanup for fixing get_super() races
On Fri, 27 Apr 2001, Linus Torvalds wrote: > > On Fri, 27 Apr 2001, Alexander Viro wrote: > > > > PS: last time I've separated that part of patch was a couple months > > ago. See if something similar to the variant below would be OK with > > you (I'll rediff it): > > This one looks fine. Erm? It _does_ pull the fsync_dev() in there (conditionally, depending on the "flag" argument of invalidate_dev()). Oh, well... Check the variant I've sent to you couple of minutes ago and tell which one you prefer, OK? Al PS: gotta love the email latency... - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] cleanup for fixing get_super() races
On Fri, 27 Apr 2001, Linus Torvalds wrote:
>
> On Fri, 27 Apr 2001, Alexander Viro wrote:
> >
> > Fine with me. Actually in _all_ cases execept cdrom.c it's preceded by
> > either sync_dev() or fsync_dev(). What do you think about pulling that
> > into the same function?
>
> I'd actually prefer not. I don't think it makes sense from a conceptual
> standpoint, and I think drivers could validly say "syncing this device at
> this point is _wrong_".
OK... That would be something like
diff -urN S4-pre8/drivers/acorn/block/mfmhd.c
S4-pre8-ream_inodes/drivers/acorn/block/mfmhd.c
--- S4-pre8/drivers/acorn/block/mfmhd.c Fri Feb 16 18:37:01 2001
+++ S4-pre8-ream_inodes/drivers/acorn/block/mfmhd.c Fri Apr 27 21:51:22 2001
@@ -1486,12 +1486,9 @@
for (i = maxp - 1; i >= 0; i--) {
int minor = start + i;
kdev_t devi = MKDEV(MAJOR_NR, minor);
- struct super_block *sb = get_super(devi);
sync_dev (devi);
- if (sb)
- invalidate_inodes (sb);
- invalidate_buffers (devi);
+ invalidate_device (devi);
mfm_gendisk.part[minor].start_sect = 0;
mfm_gendisk.part[minor].nr_sects = 0;
diff -urN S4-pre8/drivers/block/DAC960.c S4-pre8-ream_inodes/drivers/block/DAC960.c
--- S4-pre8/drivers/block/DAC960.c Thu Feb 22 06:46:26 2001
+++ S4-pre8-ream_inodes/drivers/block/DAC960.c Fri Apr 27 21:38:32 2001
@@ -5134,16 +5134,13 @@
PartitionNumber);
int MinorNumber = DAC960_MinorNumber(LogicalDriveNumber,
PartitionNumber);
- SuperBlock_T *SuperBlock = get_super(Device);
if (Controller->GenericDiskInfo.part[MinorNumber].nr_sects == 0)
continue;
/*
Flush all changes and invalidate buffered state.
*/
sync_dev(Device);
- if (SuperBlock != NULL)
- invalidate_inodes(SuperBlock);
- invalidate_buffers(Device);
+ invalidate_device(Device);
/*
Clear existing partition sizes.
*/
diff -urN S4-pre8/drivers/block/acsi.c S4-pre8-ream_inodes/drivers/block/acsi.c
--- S4-pre8/drivers/block/acsi.cFri Feb 16 22:53:03 2001
+++ S4-pre8-ream_inodes/drivers/block/acsi.cFri Apr 27 21:38:42 2001
@@ -1887,12 +1887,9 @@
for( i = max_p - 1; i >= 0 ; i-- ) {
if (gdev->part[start + i].nr_sects != 0) {
kdev_t devp = MKDEV(MAJOR_NR, start + i);
- struct super_block *sb = get_super(devp);
fsync_dev(devp);
- if (sb)
- invalidate_inodes(sb);
- invalidate_buffers(devp);
+ invalidate_device(devp);
gdev->part[start + i].nr_sects = 0;
}
gdev->part[start+i].start_sect = 0;
diff -urN S4-pre8/drivers/block/amiflop.c S4-pre8-ream_inodes/drivers/block/amiflop.c
--- S4-pre8/drivers/block/amiflop.c Fri Feb 16 18:59:20 2001
+++ S4-pre8-ream_inodes/drivers/block/amiflop.c Fri Apr 27 21:38:51 2001
@@ -1540,10 +1540,7 @@
break;
case FDFMTEND:
floppy_off(drive);
- sb = get_super(inode->i_rdev);
- if (sb)
- invalidate_inodes(sb);
- invalidate_buffers(inode->i_rdev);
+ invalidate_device(inode->i_rdev);
break;
case FDGETPRM:
memset((void *), 0, sizeof (getprm));
diff -urN S4-pre8/drivers/block/cciss.c S4-pre8-ream_inodes/drivers/block/cciss.c
--- S4-pre8/drivers/block/cciss.c Fri Apr 27 06:20:56 2001
+++ S4-pre8-ream_inodes/drivers/block/cciss.c Fri Apr 27 21:39:21 2001
@@ -694,10 +694,8 @@
for(i=max_p; i>=0; i--) {
int minor = start+i;
kdev_t devi = MKDEV(MAJOR_NR + ctlr, minor);
-struct super_block *sb = get_super(devi);
sync_dev(devi);
-if (sb) invalidate_inodes(sb);
-invalidate_buffers(devi);
+invalidate_device(devi);
gdev->part[minor].start_sect = 0;
gdev->part[minor].nr_sects = 0;
diff -urN S4-pre8/drivers/block/cpqarray.c S4-pre8-ream_inodes/drivers/block/cpqarray.c
--- S4-pre8/drivers/block/cpqarray.cFri Feb 16 22:56:28 2001
+++ S4-pre8-ream_inodes/drivers/block/cpqarray.cFri Apr 27 21:51:35 2001
@@ -1577,10 +1577,8 @@
for(i=max_p; i>=0; i--) {
int minor = start+i;
kdev_t devi = MKDEV(MAJOR_NR + ctlr, minor);
- struct super_block *sb = get_super(devi);
sync_dev(devi);
- if (sb) invalidate_inodes(sb);
- invalidate_buffers(devi);
+ invalidate_device(devi);
Re: [PATCH] cleanup for fixing get_super() races
On Fri, 27 Apr 2001, Alexander Viro wrote: > > PS: last time I've separated that part of patch was a couple months > ago. See if something similar to the variant below would be OK with > you (I'll rediff it): This one looks fine. Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] cleanup for fixing get_super() races
On Fri, 27 Apr 2001, Alexander Viro wrote: > > Fine with me. Actually in _all_ cases execept cdrom.c it's preceded by > either sync_dev() or fsync_dev(). What do you think about pulling that > into the same function? I'd actually prefer not. I don't think it makes sense from a conceptual standpoint, and I think drivers could validly say "syncing this device at this point is _wrong_". Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] cleanup for fixing get_super() races
On Fri, 27 Apr 2001, Alexander Viro wrote:
> Fine with me. Actually in _all_ cases execept cdrom.c it's preceded by
> either sync_dev() or fsync_dev(). What do you think about pulling that
> into the same function? Actually, that's what I've done in namespace
> patch (name being invalidate_dev(), BTW ;-) The only problem I see
> here is the argument telling whether we want sync or fsync (or nothing).
> OTOH, I seriously suspect that we ought replace all sync_dev() cases
> with fsync_dev() anyway... Your opinion?
> Al
PS: last time I've separated that part of patch was a couple months
ago. See if something similar to the variant below would be OK with
you (I'll rediff it):
diff -urN S2/drivers/acorn/block/mfmhd.c S2-s_lock-1/drivers/acorn/block/mfmhd.c
--- S2/drivers/acorn/block/mfmhd.c Fri Oct 27 01:35:47 2000
+++ S2-s_lock-1/drivers/acorn/block/mfmhd.c Thu Feb 22 07:00:38 2001
@@ -1486,13 +1486,8 @@
for (i = maxp - 1; i >= 0; i--) {
int minor = start + i;
kdev_t devi = MKDEV(MAJOR_NR, minor);
- struct super_block *sb = get_super(devi);
-
- sync_dev (devi);
- if (sb)
- invalidate_inodes (sb);
- invalidate_buffers (devi);
+ invalidate_dev(devi, 1);
mfm_gendisk.part[minor].start_sect = 0;
mfm_gendisk.part[minor].nr_sects = 0;
}
diff -urN S2/drivers/block/DAC960.c S2-s_lock-1/drivers/block/DAC960.c
--- S2/drivers/block/DAC960.c Thu Feb 22 06:22:36 2001
+++ S2-s_lock-1/drivers/block/DAC960.c Thu Feb 22 07:00:38 2001
@@ -5134,16 +5134,12 @@
PartitionNumber);
int MinorNumber = DAC960_MinorNumber(LogicalDriveNumber,
PartitionNumber);
- SuperBlock_T *SuperBlock = get_super(Device);
if (Controller->GenericDiskInfo.part[MinorNumber].nr_sects == 0)
continue;
/*
Flush all changes and invalidate buffered state.
*/
- sync_dev(Device);
- if (SuperBlock != NULL)
- invalidate_inodes(SuperBlock);
- invalidate_buffers(Device);
+ invalidate_dev(Device, 1);
/*
Clear existing partition sizes.
*/
diff -urN S2/drivers/block/acsi.c S2-s_lock-1/drivers/block/acsi.c
--- S2/drivers/block/acsi.c Thu Feb 22 06:22:36 2001
+++ S2-s_lock-1/drivers/block/acsi.cThu Feb 22 07:00:39 2001
@@ -1887,12 +1887,7 @@
for( i = max_p - 1; i >= 0 ; i-- ) {
if (gdev->part[start + i].nr_sects != 0) {
kdev_t devp = MKDEV(MAJOR_NR, start + i);
- struct super_block *sb = get_super(devp);
-
- fsync_dev(devp);
- if (sb)
- invalidate_inodes(sb);
- invalidate_buffers(devp);
+ invalidate_dev(devp, 2);
gdev->part[start + i].nr_sects = 0;
}
gdev->part[start+i].start_sect = 0;
diff -urN S2/drivers/block/amiflop.c S2-s_lock-1/drivers/block/amiflop.c
--- S2/drivers/block/amiflop.c Sun Oct 1 22:35:15 2000
+++ S2-s_lock-1/drivers/block/amiflop.c Thu Feb 22 07:00:39 2001
@@ -1490,7 +1490,6 @@
{
int drive = inode->i_rdev & 3;
static struct floppy_struct getprm;
- struct super_block * sb;
switch(cmd){
case HDIO_GETGEO:
@@ -1540,10 +1539,7 @@
break;
case FDFMTEND:
floppy_off(drive);
- sb = get_super(inode->i_rdev);
- if (sb)
- invalidate_inodes(sb);
- invalidate_buffers(inode->i_rdev);
+ invalidate_dev(inode->i_rdev, 0);
break;
case FDGETPRM:
memset((void *), 0, sizeof (getprm));
@@ -1677,9 +1673,6 @@
static int floppy_release(struct inode * inode, struct file * filp)
{
-#ifdef DEBUG
- struct super_block * sb;
-#endif
int drive = MINOR(inode->i_rdev) & 3;
if (unit[drive].dirty == 1) {
diff -urN S2/drivers/block/blkpg.c S2-s_lock-1/drivers/block/blkpg.c
--- S2/drivers/block/blkpg.cFri Oct 27 01:35:47 2000
+++ S2-s_lock-1/drivers/block/blkpg.c Thu Feb 22 07:00:39 2001
@@ -157,8 +157,7 @@
/* partition in use? Incomplete check for now. */
devp = MKDEV(MAJOR(dev), minor);
- if (get_super(devp) || /* mounted? */
- is_swap_partition(devp))
+ if (is_mounted(devp) || is_swap_partition(devp))
return -EBUSY;
/* all seems OK */
diff -urN S2/drivers/block/cciss.c S2-s_lock-1/drivers/block/cciss.c
--- S2/drivers/block/cciss.cThu Feb 22 06:22:36 2001
+++ S2-s_lock-1/drivers/block/cciss.c Thu Feb 22 07:00:39 2001
@@ -693,10 +693,7 @@
Re: [PATCH] cleanup for fixing get_super() races
On Fri, 27 Apr 2001, Linus Torvalds wrote: > On Fri, 27 Apr 2001, Alexander Viro wrote: > > > > Each of these places is an oopsable race with umount. We can't fix them > > without touching a lot of drivers. However, we can make the future fix > > easier if we put the above into a helper function. Patch below does that. > > I don't like the name "ream_inodes()". > > Also, a driver shouldn't know about "inodes" and "buffers". It should just > do something like > > invalidate_device(dev); > > because the only thing the driver knows about is the _device_. > > Then, invalidate_device() might do > > sb = get_super(dev) > if (sb) > invalidate_inodes (sb); > invalidate_buffers(dev); > > which makes some amount of sense. And which can later be extended to deal > with the page cache etc without the drivers ever knowing or caring. Fine with me. Actually in _all_ cases execept cdrom.c it's preceded by either sync_dev() or fsync_dev(). What do you think about pulling that into the same function? Actually, that's what I've done in namespace patch (name being invalidate_dev(), BTW ;-) The only problem I see here is the argument telling whether we want sync or fsync (or nothing). OTOH, I seriously suspect that we ought replace all sync_dev() cases with fsync_dev() anyway... Your opinion? Al - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] cleanup for fixing get_super() races
On Fri, 27 Apr 2001, Alexander Viro wrote: > > Each of these places is an oopsable race with umount. We can't fix them > without touching a lot of drivers. However, we can make the future fix > easier if we put the above into a helper function. Patch below does that. I don't like the name "ream_inodes()". Also, a driver shouldn't know about "inodes" and "buffers". It should just do something like invalidate_device(dev); because the only thing the driver knows about is the _device_. Then, invalidate_device() might do sb = get_super(dev) if (sb) invalidate_inodes (sb); invalidate_buffers(dev); which makes some amount of sense. And which can later be extended to deal with the page cache etc without the drivers ever knowing or caring. Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] cleanup for fixing get_super() races
A lot of drivers does the following:
sb = get_super(dev);
if (sb)
invalidate_inodes(sb);
Each of these places is an oopsable race with umount. We can't fix them
without touching a lot of drivers. However, we can make the future fix
easier if we put the above into a helper function. Patch below does that.
Results:
* simpler code in drivers
* drivers don't care about the superblocks and their handling
* when we will add refcounting for superblocks we will not need
to touch every block driver out there.
Patch is absolutely straightforward - function added to inode.c, declared
in fs.h and exported; instances of the body replaced with calls.
Please, apply it.
Al
diff -urN S4-pre8/drivers/acorn/block/mfmhd.c
S4-pre8-ream_inodes/drivers/acorn/block/mfmhd.c
--- S4-pre8/drivers/acorn/block/mfmhd.c Fri Feb 16 18:37:01 2001
+++ S4-pre8-ream_inodes/drivers/acorn/block/mfmhd.c Fri Apr 27 20:16:01 2001
@@ -1486,11 +1486,9 @@
for (i = maxp - 1; i >= 0; i--) {
int minor = start + i;
kdev_t devi = MKDEV(MAJOR_NR, minor);
- struct super_block *sb = get_super(devi);
sync_dev (devi);
- if (sb)
- invalidate_inodes (sb);
+ ream_inodes(devi);
invalidate_buffers (devi);
mfm_gendisk.part[minor].start_sect = 0;
diff -urN S4-pre8/drivers/block/DAC960.c S4-pre8-ream_inodes/drivers/block/DAC960.c
--- S4-pre8/drivers/block/DAC960.c Thu Feb 22 06:46:26 2001
+++ S4-pre8-ream_inodes/drivers/block/DAC960.c Fri Apr 27 20:16:31 2001
@@ -5134,15 +5134,13 @@
PartitionNumber);
int MinorNumber = DAC960_MinorNumber(LogicalDriveNumber,
PartitionNumber);
- SuperBlock_T *SuperBlock = get_super(Device);
if (Controller->GenericDiskInfo.part[MinorNumber].nr_sects == 0)
continue;
/*
Flush all changes and invalidate buffered state.
*/
sync_dev(Device);
- if (SuperBlock != NULL)
- invalidate_inodes(SuperBlock);
+ ream_inodes(Device);
invalidate_buffers(Device);
/*
Clear existing partition sizes.
diff -urN S4-pre8/drivers/block/acsi.c S4-pre8-ream_inodes/drivers/block/acsi.c
--- S4-pre8/drivers/block/acsi.cFri Feb 16 22:53:03 2001
+++ S4-pre8-ream_inodes/drivers/block/acsi.cFri Apr 27 20:16:48 2001
@@ -1887,11 +1887,9 @@
for( i = max_p - 1; i >= 0 ; i-- ) {
if (gdev->part[start + i].nr_sects != 0) {
kdev_t devp = MKDEV(MAJOR_NR, start + i);
- struct super_block *sb = get_super(devp);
fsync_dev(devp);
- if (sb)
- invalidate_inodes(sb);
+ ream_inodes(devp);
invalidate_buffers(devp);
gdev->part[start + i].nr_sects = 0;
}
diff -urN S4-pre8/drivers/block/amiflop.c S4-pre8-ream_inodes/drivers/block/amiflop.c
--- S4-pre8/drivers/block/amiflop.c Fri Feb 16 18:59:20 2001
+++ S4-pre8-ream_inodes/drivers/block/amiflop.c Fri Apr 27 20:17:09 2001
@@ -1540,9 +1540,7 @@
break;
case FDFMTEND:
floppy_off(drive);
- sb = get_super(inode->i_rdev);
- if (sb)
- invalidate_inodes(sb);
+ ream_inodes(inode->i_rdev);
invalidate_buffers(inode->i_rdev);
break;
case FDGETPRM:
diff -urN S4-pre8/drivers/block/cciss.c S4-pre8-ream_inodes/drivers/block/cciss.c
--- S4-pre8/drivers/block/cciss.c Fri Apr 27 06:20:56 2001
+++ S4-pre8-ream_inodes/drivers/block/cciss.c Fri Apr 27 20:17:30 2001
@@ -694,9 +694,8 @@
for(i=max_p; i>=0; i--) {
int minor = start+i;
kdev_t devi = MKDEV(MAJOR_NR + ctlr, minor);
-struct super_block *sb = get_super(devi);
sync_dev(devi);
-if (sb) invalidate_inodes(sb);
+ream_inodes(devi);
invalidate_buffers(devi);
gdev->part[minor].start_sect = 0;
gdev->part[minor].nr_sects = 0;
diff -urN S4-pre8/drivers/block/cpqarray.c S4-pre8-ream_inodes/drivers/block/cpqarray.c
--- S4-pre8/drivers/block/cpqarray.cFri Feb 16 22:56:28 2001
+++ S4-pre8-ream_inodes/drivers/block/cpqarray.cFri Apr 27 20:17:42 2001
@@ -1577,9 +1577,8 @@
for(i=max_p; i>=0; i--) {
int minor = start+i;
kdev_t devi = MKDEV(MAJOR_NR + ctlr, minor);
- struct super_block *sb = get_super(devi);
sync_dev(devi);
- if (sb)
Re: [PATCH] cleanup for fixing get_super() races
On Fri, 27 Apr 2001, Alexander Viro wrote: Each of these places is an oopsable race with umount. We can't fix them without touching a lot of drivers. However, we can make the future fix easier if we put the above into a helper function. Patch below does that. I don't like the name ream_inodes(). Also, a driver shouldn't know about inodes and buffers. It should just do something like invalidate_device(dev); because the only thing the driver knows about is the _device_. Then, invalidate_device() might do sb = get_super(dev) if (sb) invalidate_inodes (sb); invalidate_buffers(dev); which makes some amount of sense. And which can later be extended to deal with the page cache etc without the drivers ever knowing or caring. Linus - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] cleanup for fixing get_super() races
On Fri, 27 Apr 2001, Linus Torvalds wrote: On Fri, 27 Apr 2001, Alexander Viro wrote: Each of these places is an oopsable race with umount. We can't fix them without touching a lot of drivers. However, we can make the future fix easier if we put the above into a helper function. Patch below does that. I don't like the name ream_inodes(). Also, a driver shouldn't know about inodes and buffers. It should just do something like invalidate_device(dev); because the only thing the driver knows about is the _device_. Then, invalidate_device() might do sb = get_super(dev) if (sb) invalidate_inodes (sb); invalidate_buffers(dev); which makes some amount of sense. And which can later be extended to deal with the page cache etc without the drivers ever knowing or caring. Fine with me. Actually in _all_ cases execept cdrom.c it's preceded by either sync_dev() or fsync_dev(). What do you think about pulling that into the same function? Actually, that's what I've done in namespace patch (name being invalidate_dev(), BTW ;-) The only problem I see here is the argument telling whether we want sync or fsync (or nothing). OTOH, I seriously suspect that we ought replace all sync_dev() cases with fsync_dev() anyway... Your opinion? Al - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] cleanup for fixing get_super() races
On Fri, 27 Apr 2001, Alexander Viro wrote:
Fine with me. Actually in _all_ cases execept cdrom.c it's preceded by
either sync_dev() or fsync_dev(). What do you think about pulling that
into the same function? Actually, that's what I've done in namespace
patch (name being invalidate_dev(), BTW ;-) The only problem I see
here is the argument telling whether we want sync or fsync (or nothing).
OTOH, I seriously suspect that we ought replace all sync_dev() cases
with fsync_dev() anyway... Your opinion?
Al
PS: last time I've separated that part of patch was a couple months
ago. See if something similar to the variant below would be OK with
you (I'll rediff it):
diff -urN S2/drivers/acorn/block/mfmhd.c S2-s_lock-1/drivers/acorn/block/mfmhd.c
--- S2/drivers/acorn/block/mfmhd.c Fri Oct 27 01:35:47 2000
+++ S2-s_lock-1/drivers/acorn/block/mfmhd.c Thu Feb 22 07:00:38 2001
@@ -1486,13 +1486,8 @@
for (i = maxp - 1; i = 0; i--) {
int minor = start + i;
kdev_t devi = MKDEV(MAJOR_NR, minor);
- struct super_block *sb = get_super(devi);
-
- sync_dev (devi);
- if (sb)
- invalidate_inodes (sb);
- invalidate_buffers (devi);
+ invalidate_dev(devi, 1);
mfm_gendisk.part[minor].start_sect = 0;
mfm_gendisk.part[minor].nr_sects = 0;
}
diff -urN S2/drivers/block/DAC960.c S2-s_lock-1/drivers/block/DAC960.c
--- S2/drivers/block/DAC960.c Thu Feb 22 06:22:36 2001
+++ S2-s_lock-1/drivers/block/DAC960.c Thu Feb 22 07:00:38 2001
@@ -5134,16 +5134,12 @@
PartitionNumber);
int MinorNumber = DAC960_MinorNumber(LogicalDriveNumber,
PartitionNumber);
- SuperBlock_T *SuperBlock = get_super(Device);
if (Controller-GenericDiskInfo.part[MinorNumber].nr_sects == 0)
continue;
/*
Flush all changes and invalidate buffered state.
*/
- sync_dev(Device);
- if (SuperBlock != NULL)
- invalidate_inodes(SuperBlock);
- invalidate_buffers(Device);
+ invalidate_dev(Device, 1);
/*
Clear existing partition sizes.
*/
diff -urN S2/drivers/block/acsi.c S2-s_lock-1/drivers/block/acsi.c
--- S2/drivers/block/acsi.c Thu Feb 22 06:22:36 2001
+++ S2-s_lock-1/drivers/block/acsi.cThu Feb 22 07:00:39 2001
@@ -1887,12 +1887,7 @@
for( i = max_p - 1; i = 0 ; i-- ) {
if (gdev-part[start + i].nr_sects != 0) {
kdev_t devp = MKDEV(MAJOR_NR, start + i);
- struct super_block *sb = get_super(devp);
-
- fsync_dev(devp);
- if (sb)
- invalidate_inodes(sb);
- invalidate_buffers(devp);
+ invalidate_dev(devp, 2);
gdev-part[start + i].nr_sects = 0;
}
gdev-part[start+i].start_sect = 0;
diff -urN S2/drivers/block/amiflop.c S2-s_lock-1/drivers/block/amiflop.c
--- S2/drivers/block/amiflop.c Sun Oct 1 22:35:15 2000
+++ S2-s_lock-1/drivers/block/amiflop.c Thu Feb 22 07:00:39 2001
@@ -1490,7 +1490,6 @@
{
int drive = inode-i_rdev 3;
static struct floppy_struct getprm;
- struct super_block * sb;
switch(cmd){
case HDIO_GETGEO:
@@ -1540,10 +1539,7 @@
break;
case FDFMTEND:
floppy_off(drive);
- sb = get_super(inode-i_rdev);
- if (sb)
- invalidate_inodes(sb);
- invalidate_buffers(inode-i_rdev);
+ invalidate_dev(inode-i_rdev, 0);
break;
case FDGETPRM:
memset((void *)getprm, 0, sizeof (getprm));
@@ -1677,9 +1673,6 @@
static int floppy_release(struct inode * inode, struct file * filp)
{
-#ifdef DEBUG
- struct super_block * sb;
-#endif
int drive = MINOR(inode-i_rdev) 3;
if (unit[drive].dirty == 1) {
diff -urN S2/drivers/block/blkpg.c S2-s_lock-1/drivers/block/blkpg.c
--- S2/drivers/block/blkpg.cFri Oct 27 01:35:47 2000
+++ S2-s_lock-1/drivers/block/blkpg.c Thu Feb 22 07:00:39 2001
@@ -157,8 +157,7 @@
/* partition in use? Incomplete check for now. */
devp = MKDEV(MAJOR(dev), minor);
- if (get_super(devp) || /* mounted? */
- is_swap_partition(devp))
+ if (is_mounted(devp) || is_swap_partition(devp))
return -EBUSY;
/* all seems OK */
diff -urN S2/drivers/block/cciss.c S2-s_lock-1/drivers/block/cciss.c
--- S2/drivers/block/cciss.cThu Feb 22 06:22:36 2001
+++ S2-s_lock-1/drivers/block/cciss.c Thu Feb 22 07:00:39 2001
@@ -693,10 +693,7 @@
Re: [PATCH] cleanup for fixing get_super() races
On Fri, 27 Apr 2001, Alexander Viro wrote: Fine with me. Actually in _all_ cases execept cdrom.c it's preceded by either sync_dev() or fsync_dev(). What do you think about pulling that into the same function? I'd actually prefer not. I don't think it makes sense from a conceptual standpoint, and I think drivers could validly say syncing this device at this point is _wrong_. Linus - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] cleanup for fixing get_super() races
On Fri, 27 Apr 2001, Alexander Viro wrote: PS: last time I've separated that part of patch was a couple months ago. See if something similar to the variant below would be OK with you (I'll rediff it): This one looks fine. Linus - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] cleanup for fixing get_super() races
On Fri, 27 Apr 2001, Linus Torvalds wrote:
On Fri, 27 Apr 2001, Alexander Viro wrote:
Fine with me. Actually in _all_ cases execept cdrom.c it's preceded by
either sync_dev() or fsync_dev(). What do you think about pulling that
into the same function?
I'd actually prefer not. I don't think it makes sense from a conceptual
standpoint, and I think drivers could validly say syncing this device at
this point is _wrong_.
OK... That would be something like
diff -urN S4-pre8/drivers/acorn/block/mfmhd.c
S4-pre8-ream_inodes/drivers/acorn/block/mfmhd.c
--- S4-pre8/drivers/acorn/block/mfmhd.c Fri Feb 16 18:37:01 2001
+++ S4-pre8-ream_inodes/drivers/acorn/block/mfmhd.c Fri Apr 27 21:51:22 2001
@@ -1486,12 +1486,9 @@
for (i = maxp - 1; i = 0; i--) {
int minor = start + i;
kdev_t devi = MKDEV(MAJOR_NR, minor);
- struct super_block *sb = get_super(devi);
sync_dev (devi);
- if (sb)
- invalidate_inodes (sb);
- invalidate_buffers (devi);
+ invalidate_device (devi);
mfm_gendisk.part[minor].start_sect = 0;
mfm_gendisk.part[minor].nr_sects = 0;
diff -urN S4-pre8/drivers/block/DAC960.c S4-pre8-ream_inodes/drivers/block/DAC960.c
--- S4-pre8/drivers/block/DAC960.c Thu Feb 22 06:46:26 2001
+++ S4-pre8-ream_inodes/drivers/block/DAC960.c Fri Apr 27 21:38:32 2001
@@ -5134,16 +5134,13 @@
PartitionNumber);
int MinorNumber = DAC960_MinorNumber(LogicalDriveNumber,
PartitionNumber);
- SuperBlock_T *SuperBlock = get_super(Device);
if (Controller-GenericDiskInfo.part[MinorNumber].nr_sects == 0)
continue;
/*
Flush all changes and invalidate buffered state.
*/
sync_dev(Device);
- if (SuperBlock != NULL)
- invalidate_inodes(SuperBlock);
- invalidate_buffers(Device);
+ invalidate_device(Device);
/*
Clear existing partition sizes.
*/
diff -urN S4-pre8/drivers/block/acsi.c S4-pre8-ream_inodes/drivers/block/acsi.c
--- S4-pre8/drivers/block/acsi.cFri Feb 16 22:53:03 2001
+++ S4-pre8-ream_inodes/drivers/block/acsi.cFri Apr 27 21:38:42 2001
@@ -1887,12 +1887,9 @@
for( i = max_p - 1; i = 0 ; i-- ) {
if (gdev-part[start + i].nr_sects != 0) {
kdev_t devp = MKDEV(MAJOR_NR, start + i);
- struct super_block *sb = get_super(devp);
fsync_dev(devp);
- if (sb)
- invalidate_inodes(sb);
- invalidate_buffers(devp);
+ invalidate_device(devp);
gdev-part[start + i].nr_sects = 0;
}
gdev-part[start+i].start_sect = 0;
diff -urN S4-pre8/drivers/block/amiflop.c S4-pre8-ream_inodes/drivers/block/amiflop.c
--- S4-pre8/drivers/block/amiflop.c Fri Feb 16 18:59:20 2001
+++ S4-pre8-ream_inodes/drivers/block/amiflop.c Fri Apr 27 21:38:51 2001
@@ -1540,10 +1540,7 @@
break;
case FDFMTEND:
floppy_off(drive);
- sb = get_super(inode-i_rdev);
- if (sb)
- invalidate_inodes(sb);
- invalidate_buffers(inode-i_rdev);
+ invalidate_device(inode-i_rdev);
break;
case FDGETPRM:
memset((void *)getprm, 0, sizeof (getprm));
diff -urN S4-pre8/drivers/block/cciss.c S4-pre8-ream_inodes/drivers/block/cciss.c
--- S4-pre8/drivers/block/cciss.c Fri Apr 27 06:20:56 2001
+++ S4-pre8-ream_inodes/drivers/block/cciss.c Fri Apr 27 21:39:21 2001
@@ -694,10 +694,8 @@
for(i=max_p; i=0; i--) {
int minor = start+i;
kdev_t devi = MKDEV(MAJOR_NR + ctlr, minor);
-struct super_block *sb = get_super(devi);
sync_dev(devi);
-if (sb) invalidate_inodes(sb);
-invalidate_buffers(devi);
+invalidate_device(devi);
gdev-part[minor].start_sect = 0;
gdev-part[minor].nr_sects = 0;
diff -urN S4-pre8/drivers/block/cpqarray.c S4-pre8-ream_inodes/drivers/block/cpqarray.c
--- S4-pre8/drivers/block/cpqarray.cFri Feb 16 22:56:28 2001
+++ S4-pre8-ream_inodes/drivers/block/cpqarray.cFri Apr 27 21:51:35 2001
@@ -1577,10 +1577,8 @@
for(i=max_p; i=0; i--) {
int minor = start+i;
kdev_t devi = MKDEV(MAJOR_NR + ctlr, minor);
- struct super_block *sb = get_super(devi);
sync_dev(devi);
- if (sb) invalidate_inodes(sb);
- invalidate_buffers(devi);
+ invalidate_device(devi);
Re: [PATCH] cleanup for fixing get_super() races
On Fri, 27 Apr 2001, Linus Torvalds wrote: On Fri, 27 Apr 2001, Alexander Viro wrote: PS: last time I've separated that part of patch was a couple months ago. See if something similar to the variant below would be OK with you (I'll rediff it): This one looks fine. Erm? It _does_ pull the fsync_dev() in there (conditionally, depending on the flag argument of invalidate_dev()). Oh, well... Check the variant I've sent to you couple of minutes ago and tell which one you prefer, OK? Al PS: gotta love the email latency... - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
