Date:	Thu, 17 Jul 2014 18:45:37 +0300
From:	Timofey Titovets <nefelim4ag@...il.com>
To:	Jerome Marchand <jmarchan@...hat.com>,
	Sergey Senozhatsky <sergey.senozhatsky@...il.com>
CC:	minchan@...nel.org, linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Nitin Gupta <ngupta@...are.org>
Subject: Re: [PATCH v2] zram: auto add/del devices on demand



On 07/17/2014 06:26 PM, Jerome Marchand wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 07/17/2014 05:01 PM, Timofey Titovets wrote:
>>
>>
>> On 07/17/2014 05:04 PM, Sergey Senozhatsky wrote:
>>> On (07/17/14 15:27), Timofey Titovets wrote:
>>>> This add supporting of autochange count of zram devices on
>>>> demand, like loop devices; This working by following rules: -
>>>> Always save minimum devices count specified by num_device (can
>>>> be specified while kernel module loading) - if last device
>>>> already using add new device; - if last and prelast devices is
>>>> free - delete last zram device;
>>>>
>>>> Signed-off-by: Timofey Titovets <nefelim4ag@...il.com>
>>>>
>>>>  From v1 -> v2: Delete useless variable 'ret', added
>>>> documentation for explain new zram behaviour
>>>>
>>>> Please pull from: https://github.com/Nefelim4ag/linux.git ---
>>>> Tested on 3.15.5-2-ARCH, can be applied on any kernel version
>>>> after this patch 'zram: add LZ4 compression support' -
>>>> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=6e76668e415adf799839f0ab205142ad7002d260
>>>>
>>>>
>>>>
> - ---
>>>> diff --git a/Documentation/blockdev/zram.txt
>>>> b/Documentation/blockdev/zram.txt index 0595c3f..7f5c921
>>>> 100644 --- a/Documentation/blockdev/zram.txt +++
>>>> b/Documentation/blockdev/zram.txt @@ -18,9 +18,19 @@ Following
>>>> shows a typical sequence of steps for using zram.
>>>>
>>>> 1) Load Module: modprobe zram num_devices=4 -    This creates 4
>>>> devices: /dev/zram{0,1,2,3} +    This pre creates 4 devices:
>>>> /dev/zram{0,1,2,3} (num_devices parameter is optional. Default:
>>>> 1)
>>>>
>>>> +    Kernel dynamically changes number of zram devices by
>>>> demand +    (between num_devices and 31) +    If set disk
>>>> size(4) for last device +    kernel automatically adds new zram
>>>> device +    If last two devices have zero disk size after
>>>> reset(8), +    kernel will destroy last device + +    Summing
>>>> up all features, comes one simple rule: +    "The last zram
>>>> device is always free for use"
>>>
>>> I can't understand what is the real benefit?
>>>
>>>
>>>> 2) Set max number of compression streams Compression backend
>>>> may use up to max_comp_streams compression streams, thus
>>>> allowing up to max_comp_streams concurrent compression
>>>> operations. diff --git a/drivers/block/zram/zram_drv.c
>>>> b/drivers/block/zram/zram_drv.c index 089e72c..9b2fc89 100644
>>>> --- a/drivers/block/zram/zram_drv.c +++
>>>> b/drivers/block/zram/zram_drv.c @@ -42,6 +42,10 @@ static const
>>>> char *default_compressor = "lzo";
>>>>
>>>> /* Module params (documentation at end) */ static unsigned int
>>>> num_devices = 1; +static unsigned int last_created_dev = 1; +
>>>> +static void zram_add_dev(void); +static void
>>>> zram_del_dev(void); #define ZRAM_ATTR_RO(name)
>>>> \ static ssize_t zram_attr_##name##_show(struct device *d,
>>>> \ @@ -168,6 +172,7 @@ static ssize_t
>>>> comp_algorithm_store(struct device *dev, struct
>>>> device_attribute *attr, const char *buf, size_t len) { struct
>>>> zram *zram = dev_to_zram(dev); + down_write(&zram->init_lock);
>>>> if (init_done(zram)) { up_write(&zram->init_lock); @@ -239,6
>>>> +244,7 @@ static struct zram_meta *zram_meta_alloc(u64
>>>> disksize) { size_t num_pages; struct zram_meta *meta =
>>>> kmalloc(sizeof(*meta), GFP_KERNEL); + if (!meta) goto out;
>>>>
>>>> @@ -374,6 +380,7 @@ static int zram_bvec_read(struct zram
>>>> *zram, struct bio_vec *bvec, struct page *page; unsigned char
>>>> *user_mem, *uncmem = NULL; struct zram_meta *meta =
>>>> zram->meta; + page = bvec->bv_page;
>>>>
>>>> read_lock(&meta->tb_lock); @@ -607,6 +614,7 @@ static void
>>>> zram_reset_device(struct zram *zram, bool reset_capacity) /*
>>>> Free all pages that are still in this zram device */ for (index
>>>> = 0; index < zram->disksize >> PAGE_SHIFT; index++) { unsigned
>>>> long handle = meta->table[index].handle; + if (!handle)
>>>> continue;
>>>>
>>>> @@ -668,6 +676,7 @@ static ssize_t disksize_store(struct device
>>>> *dev, set_capacity(zram->disk, zram->disksize >>
>>>> SECTOR_SHIFT); revalidate_disk(zram->disk);
>>>> up_write(&zram->init_lock); +    zram_add_dev(); return len;
>>>>
>>>> out_destroy_comp: @@ -712,6 +721,7 @@ static ssize_t
>>>> reset_store(struct device *dev, bdput(bdev);
>>>>
>>>> zram_reset_device(zram, true); +    zram_del_dev(); return
>>>> len;
>>>>
>>>> out: @@ -954,6 +964,41 @@ static void destroy_device(struct
>>>> zram *zram) blk_cleanup_queue(zram->queue); }
>>>>
>>>> +/* remove last free device, if last and prelast dev a free */
>>>> +static void zram_del_dev(void) +{ +    if (last_created_dev <
>>>> num_devices) +        return; + +    if
>>>> (zram_devices[last_created_dev].disksize == 0 && +
>>>> zram_devices[last_created_dev-1].disksize == 0 +        ) {
>>>
>>> racy?
>>>
>>>> +        destroy_device(&zram_devices[last_created_dev]); +
>>>> last_created_dev--; +        pr_info("Deleted zram%u device\n",
>>>> last_created_dev); +    } +} + +/* Auto add empty zram device,
>>>> if last device in use */ +static void zram_add_dev(void) +{ +
>>>> if (last_created_dev+1 > max_num_devices) { +
>>>> pr_warn("Can't add zram%u, max device number reached\n", +
>>>> num_devices); +        return; +    }
>>>
>>> racy?
>>>
>>>> +    if (&zram_devices[last_created_dev].disksize > 0) { +
>>>> last_created_dev++; +        if
>>>> (create_device(&zram_devices[last_created_dev],
>>>> last_created_dev)) { +
>>>> destroy_device(&zram_devices[last_created_dev]); +
>>>> last_created_dev--;
>>>
>>> racy?
>>>
>>>
>>> -ss
>>>
>>>
>>>> +            return; +        } +        pr_info("Created
>>>> zram%u device\n", last_created_dev); +    } +} + static int
>>>> __init zram_init(void) { int ret, dev_id; @@ -972,18 +1017,20
>>>> @@ static int __init zram_init(void) goto out; }
>>>>
>>>> -    /* Allocate the device array and initialize each one */ -
>>>> zram_devices = kzalloc(num_devices * sizeof(struct zram),
>>>> GFP_KERNEL); +    /* Allocate the device array */ +
>>>> zram_devices = kcalloc(max_num_devices, sizeof(struct zram),
>>>> GFP_KERNEL); if (!zram_devices) { ret = -ENOMEM; goto
>>>> unregister; }
>>>>
>>>> +    /* Initialise zram{0..num_devices} */ for (dev_id = 0;
>>>> dev_id < num_devices; dev_id++) { ret =
>>>> create_device(&zram_devices[dev_id], dev_id); if (ret) goto
>>>> free_devices; } +    last_created_dev = num_devices-1;
>>>>
>>>> pr_info("Created %u device(s) ...\n", num_devices);
>>>>
>>>> @@ -1004,7 +1051,7 @@ static void __exit zram_exit(void) int
>>>> i; struct zram *zram;
>>>>
>>>> -    for (i = 0; i < num_devices; i++) { +    for (i = 0; i <
>>>> last_created_dev+1; i++) { zram = &zram_devices[i];
>>>>
>>>> destroy_device(zram); @@ -1025,7 +1072,7 @@
>>>> module_init(zram_init); module_exit(zram_exit);
>>>>
>>>> module_param(num_devices, uint, 0);
>>>> -MODULE_PARM_DESC(num_devices, "Number of zram devices");
>>>> +MODULE_PARM_DESC(num_devices, "Number of pre created  zram
>>>> devices");
>>>>
>>>> MODULE_LICENSE("Dual BSD/GPL"); MODULE_AUTHOR("Nitin Gupta
>>>> <ngupta@...are.org>");
>>>>
>>
>> I can't understand what do you mean on "racy?"
>
> Racy as in "race condition".
>
>> if you mind random empty lines in code, I've just checked code by
>> checkpatch.pl and fixed warnings.
>>
>> I'll try to explain, why I wrote a patch: zram, as a loop device,
>> is a virtual device, not physical, and i think that adding new free
>> devices automatically (like loop) is useful and expected behavior
>> (for virtual device).
>
> I don't know much about loop devices. How are they dynamically
> added/deleted? Is that the same mechanism as your patch implement?
> That may be a matter of taste, but I personally find that behaviour
> rather awkward.
>
> Jerome
>

loop device has self logic, as i understand it's working through 
loop-control and losetup.
I will try to understand clearer how loop devices are working and 
rewrite my patch.
I think if in final i'll write clear, not buggy patch, it can be applied 
on kernel, but now my code is crap and i must rewrite it =).

Anyway thanks for reviewing.

Timofey

>>
>> By default, zram creates only one device, and if I need to use two
>> devices (for example), my actions lead to module’s reloading: 1.
>> umount busy device 2. lose my data. Yes, I can backup it, and
>> restore, but this is also useless, if I can do it more beautiful 3.
>> unload module 4. load module again with new num devices parameter
>> or make it parametre "permanent" by creating file in modprobe.d
>>
>> This is handwork and such behavior is uncomfortable.
>>
>> if number of devices is dynamically changeable, I can skip this
>> actions and save my time and energy instead of wasting on these
>> activities. -- To unsubscribe from this list: send the line
>> "unsubscribe linux-kernel" in the body of a message to
>> majordomo@...r.kernel.org More majordomo info at
>> http://vger.kernel.org/majordomo-info.html Please read the FAQ at
>> http://www.tux.org/lkml/
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
>
> iQEcBAEBAgAGBQJTx+s4AAoJEHTzHJCtsuoClfkIAKYfuhWQWSvSD9XPQUD0R7HI
> kW6865EWpNZjWR5WVYng48dvYwPV+2YGKJE1a+FyIwa06jjDO+Gm/FnOcnUhUcDz
> NFjTSBa1CIbh1hXU5uiI6u3BNw66BDjaprVe6x1tcW96y7T7Po+IWdf5+N7GMCtf
> 9H14KOCd20X5FsvPz70UkkpkLQZK1UdzmQ+1KYPb+bC3ACgTsuxs9Wxs1Tzn+tP/
> DDvbZRd4FAcGZsC88U2gyuNAhoVYXVqzokukeV7hRlsSCFibwEOQzoZl/xljyICK
> Xv+uuMupmAVWv9CrlYexqT/HFoDOhAs7uRM2sgza39apUdQfkGg0vIw+9GV7+Q0=
> =Bxhk
> -----END PGP SIGNATURE-----
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists