Re: [patch] scsi: revert "[SCSI] Get rid of scsi_cmnd->done"

Linus Torvalds
2008-01-06T18:23:05Z

On Sun, 6 Jan 2008, James Bottomley wrote:
> > index 993f78c..6a20da9 100644
> > --- a/fs/block_dev.c
> > +++ b/fs/block_dev.c
> > @@ -1191,6 +1191,7 @@ static int do_open(struct block_device *bdev, struct file *file, int for_part)
> > }
> > if (bdev->bd_invalidated)
> > rescan_partitions(bdev->bd_disk, bdev);
> > + bd_inode->i_size = (loff_t)get_capacity(disk)<<9;
> > }
> > }
> > bdev->bd_openers++;
>
> Actually, I think that code is fine ... the block size shouldn't change
> across positive values of bd_openers.

The block size should indeed not change, and that's why we must *not* do
bd_set_size() there (because that changes the block size too, not just the
size of the device).

But I would argue that if we have had a device invalidation event (ie
media change), then we should indeed change the actual *size* of the block
device as seen by anybody who opens the file again.

And yes, it will affect old openers of the same inode too (since the size
is in the inode, not the file descriptor), but considering that this
really should only happen for media change events, I think that's better
than what we used to have.

Now, we could have made it even more clear by moving the "i_size" setting
into the same "if (dbev->bd_invalidated)" conditional, but the thing is,
we only set bd_invalidated for devices that have minors, so things like
floppies etc that don't have partition support also don't have
bd_invalidated set (in effect, bd_invalidated really *is* just a flag for
the partitioning code, not for disk change in general).

That said:

> pktcdvd shouldn't be mucking with the size of the underlying CD/DVD ...

I'm not sure if it should be mucking with the size or not, but it
definitely shouldn't be mucking with the block-size, because that can
indeed cause huge problems.

So removing the bd_set_size() is almost certainly the right thing to do
(because it's always incorrect to do at an "inner" open), and the real
size should have already been set by the regular open.

But this should be tested by somebody who uses the dang thing. My personal
favourite for a real fix would actually be to always make the capacity of
the pktcdvd device match the capacity of the underlying device, ie the
diff would look something like the appended (untested as usual), but that
may be a bit too extensive a change.

But regardless, I think the i_size change makes sense - at least in some
form. It doesn't necessarily have to be the explicit i_size setting: I
also considered just changing the block_dev.c do_open() make bd_set_size()
_unconditionally_, and then move the "if (!bdev->bd_openers)" test into
bd_set_size(), so that people couldn't even change the blocksize by
mistake!

I'd still like to hear comments from Al in particular, if he has something
to say.

Linus
---
drivers/block/pktcdvd.c | 4 +---
1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index 3535ef8..1b23681 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -2342,9 +2342,7 @@ static int pkt_open_dev(struct pktcdvd_device *pd, int write)
goto out_unclaim;
}

- set_capacity(pd->disk, lba << 2);
- set_capacity(pd->bdev->bd_disk, lba << 2);
- bd_set_size(pd->bdev, (loff_t)lba << 11);
+ set_capacity(pd->disk, get_capacity(pd->bdev->bd_disk));

q = bdev_get_queue(pd->bdev);
if (write) {