On Wed, 5 May 2010, Nitin Gupta wrote:
>
> Please see the original mail below (patch you nacked). Maybe, at that time, I didn't
> make it clear that ramzswap is really a *block device* :)
Oh, you're right. I looked up another patch of yours that had that
swap_info" thing, and decided I hated that one with a passion, but it's
not the one I NAK'ed in the earlier discussion.
Now that I see the block-layer patch, my reaction is (a) it's so much
nicer than using that horrid nasty 'notifier' crap and (b) it reminds me
why I wasn't entirely happy: it doesn't work - or even make sense - for
filesystem-backed swap.
So when you do
struct gendisk *disk = p->bdev->bd_disk;
..
if (disk->fops->swap_slot_free_notify)
disk->fops->swap_slot_free_notify(p->bdev, offset);
there's nothing that says that 'offset' makes any sense at all, because if
it's a swap-file on a device, it does all kinds of totally wrong things.
So I don't think that patch works either. I still suspect that the right
"level" for something like this should be the 'mapping' level (which is
how we actually do the write), but that seems to not work well with the
block device layer.
So at a _minimum_, that 'disk->fops' approach needs to verify that the
swap device is actually the whole bdev, and that the bdev isn't just the
backing store for a swap _file_.
I dunno how to best check that. Either add a new flag to
'swap_info_struct' that gets set on 'swapon()' whether it's a full device
or a file. Or possibly just something like
static int swap_is_block_device(struct swap_info_struct *p)
{
return S_ISBLK(p->swap_file->f_mapping->host);
}
instead.
Because doing that 'disk->fops' thing _really_ isn't right if it isn't a
disk.
Linus
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
