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

[Linus Torvalds]

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) {
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/