Prev: trace/trace_output.c: fix "integer as NULL pointer" warning.
Next: fat: BKL ioctl pushdown
From: Nitin Gupta on 5 May 2010 12:10
On 05/05/2010 08:44 PM, Linus Torvalds wrote:
>
>
> On Wed, 5 May 2010, Nitin Gupta wrote:
>>
>> ramzswap driver creates RAM based block devices which can be
>> used (only) as swap disks. Pages swapped to these disks are
>> compressed and stored in memory itself.
>
> Ok, this patch series looks way better, if only because it looks less
> hacky.
>
> That said, I absolutely _hate_ the f*cking notifier model that takes
> "type" flags. It's a disgrace. It's a horrible horrible model.
>

You mean you didn't like the 'swap type' value passed around by notifier
calls, as here:
"blocking_notifier_call_chain(&swapon_notify_list, type, swap_file);" ?

> I'd much rather bind a nice "swap_operations" structure to the device, and
> have that structure have function pointers for the different operations.
> No stupid "operation type codes". Real, honest-to-goodness function
> pointers.
>

I think such 'swap_operations' structure will be have to be part of
block_device_operations, so we may access it from swap_entry_free()
where a swap slot is freed. This will also get rid of all this notifier
stuff.

The patch you nacked did something similar: it add 'swap_slot_free_callback'
directly to block_device_operations. Without such change, I could not think
of any way to do away with notifiers.


> The notifier layer is a total piece of sh*t. I'm sorry I ever merged it,
> and I'm _doubly_ sorry that it's use is so horribly widespread. It's a
> mistake.

Thanks,
Nitin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Nitin Gupta on 5 May 2010 13:00
On 05/05/2010 09:52 PM, Linus Torvalds wrote:
>
>
> On Wed, 5 May 2010, Nitin Gupta wrote:
>>
>> I think such 'swap_operations' structure will be have to be part of
>> block_device_operations, so we may access it from swap_entry_free()
>> where a swap slot is freed. This will also get rid of all this notifier
>> stuff.
>
> Yes, I think adding it to block_device_operations would be fine. That
> sounds like a sane layering, and would make it easy for a block device
> driver to say "I want to know about swap events".
>
> In fact, for regular block devices, a swap block free might well translate
> into a TRIM command some day (where "some day" means when the SSD's
> actually get their stuff together and there is real upside and not just
> "most cases will be very slow and the upside is debatable").
>

Its great if adding such a callback to block_device_operations is okay. Hugh
suggested this approach and I'm distributing it with compcache for quite some
time now:
http://code.google.com/p/compcache/source/browse/patches/patch_swap_notify_core_support_2.6.33.diff

Can you please have a lot at patch above and see if its acceptable? Then I will
post it to lkml again.


>> The patch you nacked did something similar: it add 'swap_slot_free_callback'
>> directly to block_device_operations. Without such change, I could not think
>> of any way to do away with notifiers.
>
> Umm. No. IIRC, the patch I NAK'ed aded it to the 'swap_info_struct', which
> I said was the wrong level. The block device driver level would seem to be
> the _right_ level, since that's what ramzswap is. No?
>
> Also, the patch I NAK'ed also used those nasty notifier chains, making it
> even uglier.

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* :)



-------- Original Message --------
Subject: [nacked] mm-add-swap-slot-free-callback-to-block_device_operations.patch removed from -mm tree
Date: Tue, 09 Mar 2010 14:30:27 -0800
From: akpm(a)linux-foundation.org
To: <snip/>

The patch titled
mm: add swap slot free callback to block_device_operations
has been removed from the -mm tree. Its filename was
mm-add-swap-slot-free-callback-to-block_device_operations.patch

This patch was dropped because it was nacked

The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/

------------------------------------------------------
Subject: mm: add swap slot free callback to block_device_operations
From: Nitin Gupta <ngupta(a)vflare.org>

This callback is required when RAM based devices are used as swap disks.
One such device is ramzswap[1] which is used as compressed in-memory swap
disk. For such devices, we need a callback as soon as a swap slot is no
longer used to allow freeing memory allocated for this slot. Without this
callback, stale data can quickly accumulate in memory defeating the whole
purpose of such devices.

Another user of this callback will be "preswap" as introduced by
"Transcendent Memory" patches: http://lwn.net/Articles/367286/ (I intend
to integrade preswap with ramzswap).

[1] ramzswap: http://code.google.com/p/compcache/

Signed-off-by: Nitin Gupta <ngupta(a)vflare.org>
Signed-off-by: Hugh Dickins <hugh.dickins(a)tiscali.co.uk>
Acked-by: Hugh Dickins <hugh.dickins(a)tiscali.co.uk>
Cc: Greg KH <greg(a)kroah.com>
Cc: Pekka Enberg <penberg(a)cs.helsinki.fi>
Cc: Al Viro <viro(a)ZenIV.linux.org.uk>
Cc: Christoph Hellwig <hch(a)infradead.org>
Cc: Jens Axboe <jens.axboe(a)oracle.com>
Cc: Andi Kleen <andi(a)firstfloor.org>
Signed-off-by: Andrew Morton <akpm(a)linux-foundation.org>
---

include/linux/blkdev.h | 2 ++
mm/swapfile.c | 3 +++
2 files changed, 5 insertions(+)

diff -puN include/linux/blkdev.h~mm-add-swap-slot-free-callback-to-block_device_operations include/linux/blkdev.h
--- a/include/linux/blkdev.h~mm-add-swap-slot-free-callback-to-block_device_operations
+++ a/include/linux/blkdev.h
@@ -1310,6 +1310,8 @@ struct block_device_operations {
unsigned long long);
int (*revalidate_disk) (struct gendisk *);
int (*getgeo)(struct block_device *, struct hd_geometry *);
+ /* this callback is with swap_lock and sometimes page table lock held */
+ void (*swap_slot_free_notify) (struct block_device *, unsigned long);
struct module *owner;
};

diff -puN mm/swapfile.c~mm-add-swap-slot-free-callback-to-block_device_operations mm/swapfile.c
--- a/mm/swapfile.c~mm-add-swap-slot-free-callback-to-block_device_operations
+++ a/mm/swapfile.c
@@ -574,6 +574,7 @@ static unsigned char swap_entry_free(str

/* free if no reference */
if (!usage) {
+ struct gendisk *disk = p->bdev->bd_disk;
if (offset < p->lowest_bit)
p->lowest_bit = offset;
if (offset > p->highest_bit)
@@ -583,6 +584,8 @@ static unsigned char swap_entry_free(str
swap_list.next = p->type;
nr_swap_pages++;
p->inuse_pages--;
+ if (disk->fops->swap_slot_free_notify)
+ disk->fops->swap_slot_free_notify(p->bdev, offset);
}

return usage;
_

Patches currently in -mm which might be from ngupta(a)vflare.org are

linux-next.patch
mm-add-swap-slot-free-callback-to-block_device_operations.patch


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: trace/trace_output.c: fix "integer as NULL pointer" warning.
Next: fat: BKL ioctl pushdown