pkoutoupis / rapiddisk

An Advanced Linux RAM Drive and Caching kernel modules. Dynamically allocate RAM as block devices. Use them as stand alone drives or even map them as caching nodes to slower local disk drives. Access those volumes locally or export them across an NVMe Target network. Manage it all from a web API.
http://www.rapiddisk.org
GNU General Public License v2.0
298 stars 49 forks source link

During the 'resize' operation, it may happens buggy messages are printed #142

Closed matteotenca closed 1 year ago

matteotenca commented 1 year ago

Hi Petros,

I noticed this small problem while working on #139.

In rdsk.c at this point:

https://github.com/pkoutoupis/rapiddisk/blob/d6d89a9104ea15df0b3adc84f6b9d35b720246c0/src/rdsk.c#L518-L521

that ioctl call is used to get maxsectors value, which later on is used as if it represents the nominal ramdisk size. But this is not the case, since IOCTL_RD_GET_STATS returns the maxium number of sectors ever allocated/used - thus, if the ramdisk was created and never used, maxsectors will be equal to 0.

Soon after, in this piece of code:

https://github.com/pkoutoupis/rapiddisk/blob/d6d89a9104ea15df0b3adc84f6b9d35b720246c0/src/rdsk.c#L525-L528

the maxsectors value is used to perform sanity checks on the size the user specified to resize the ramdisk to.

if ((((size * 1024 * 1024) / BYTES_PER_BLOCK) <= (max_sectors)) || ((size * 1024) == (rd_size / 1024)))

Splitting the expression:

(size * 1024 * 1024) / BYTES_PER_BLOCK) <= (max_sectors)

Will always be false unless the user specified a resize target (stored in size in MB) == 0. On the other side, if the user specified a new size equal to the current one (stored in rd_size), the second test will be true and an error message is printed:

printf("Error. Please specify a size larger than %llu Mbytes\n", (((max_sectors * BYTES_PER_BLOCK) / 1024) / 1024));

Since max_sectors will be 0 if the ramdisk was just created, the error messages will be:

Error. Please specify a size larger than 0 Mbytes

Furthermore, this approach allows the user to downsize a ramdisk, let's say, created 100 MB big, to, for example, 1 MB, if no data were ever written on it. In other words, the user can downsize the ramdisk to a dimension which have to be greater than the maximum sectors ever used, and not greater of the creation size. This may be a wanted behaviour, I am not sure.

The ioctl call IOCTL_RD_GET_STATS refers to this piece of code:

https://github.com/pkoutoupis/rapiddisk/blob/d6d89a9104ea15df0b3adc84f6b9d35b720246c0/module/rapiddisk.c#L719-L722

The structure referred to is defined as:

https://github.com/pkoutoupis/rapiddisk/blob/d6d89a9104ea15df0b3adc84f6b9d35b720246c0/module/rapiddisk.c#L71-L84

And your comment states clearly that

max_blk_alloc;  /* rdsk: to keep track of highest sector write  */

So, new ramdisk == no writes -> max_blk_alloc == 0, if I understood correctly.

Some examples, compiled using current master branch under:

Distributor ID: Ubuntu
Description:    Ubuntu 22.04.1 LTS
Release:        22.04
Codename:       jammy

Kernel: 5.15.0-56-generic

Creation of a 100 MB ramdisk:

shub@ubuserver:~/rapiddisk_clion_2_debug_new_branch/src$ sudo ./rapiddisk -a 100
rapiddisk 8.2.0
Copyright 2011 - 2022 Petros Koutoupis

Attached device rd0 of size 100 Mbytes
shub@ubuserver:~/rapiddisk_clion_2_debug_new_branch/src$ sudo ./rapiddisk -l
rapiddisk 8.2.0
Copyright 2011 - 2022 Petros Koutoupis

List of RapidDisk device(s):

 RapidDisk Device 1: rd0        Size (KB): 102400       Usage (KB): 0   Status: Unlocked

List of RapidDisk-Cache mapping(s):

  None

Usage is == 0 KB since it was never filled. Resize the ramdisk to 1 MB:

shub@ubuserver:~/rapiddisk_clion_2_debug_new_branch/src$ sudo ./rapiddisk -r rd0 -c 1
rapiddisk 8.2.0
Copyright 2011 - 2022 Petros Koutoupis

Resized device rd0 to 1 Mbytes
shub@ubuserver:~/rapiddisk_clion_2_debug_new_branch/src$ sudo ./rapiddisk -l
rapiddisk 8.2.0
Copyright 2011 - 2022 Petros Koutoupis

List of RapidDisk device(s):

 RapidDisk Device 1: rd0        Size (KB): 1024 Usage (KB): 0   Status: Unlocked

List of RapidDisk-Cache mapping(s):

  None

shub@ubuserver:~/rapiddisk_clion_2_debug_new_branch/src$

The ramdisk size drecreased to 1 MB, usage is still 0 KB.

Resize the ramdisk to 10 MB and to 10 MB again - notice the error message.

shub@ubuserver:~/rapiddisk_clion_2_debug_new_branch/src$ sudo ./rapiddisk -r rd0 -c 10
rapiddisk 8.2.0
Copyright 2011 - 2022 Petros Koutoupis

Resized device rd0 to 10 Mbytes
shub@ubuserver:~/rapiddisk_clion_2_debug_new_branch/src$ sudo ./rapiddisk -r rd0 -c 10
rapiddisk 8.2.0
Copyright 2011 - 2022 Petros Koutoupis

Error. Please specify a size larger than 0 Mbytes
shub@ubuserver:~/rapiddisk_clion_2_debug_new_branch/src$

This behaviour was left unchanged in #139.

pkoutoupis commented 1 year ago

This should fix it. I was not checking a specific case originally. Nice catch!

diff --git a/src/rdsk.c b/src/rdsk.c
index edc8d94..455876b 100644
--- a/src/rdsk.c
+++ b/src/rdsk.c
@@ -826,8 +826,13 @@ int mem_device_resize(struct RD_PROFILE *prof, char *string, unsigned long long
        close(fd);

        if ((((size * 1024 * 1024) / BYTES_PER_BLOCK) <= (max_sectors)) || ((size * 1024) == (rd_size / 1024))) {
-               msg = "Error. Please specify a size larger than %llu Mbytes";
-               print_error(msg, return_message, (((max_sectors * BYTES_PER_BLOCK) / 1024) / 1024));
+               if ((size * 1024) == (rd_size / 1024)) {
+                       msg = "Error. Size is currently set to %llu Mbytes. Please specify a size larger than %llu Mbytes.";
+                       print_error(msg, return_message, (rd_size / 1024) / 1024, (((max_sectors * BYTES_PER_BLOCK) / 1024) / 1024));
+               } else {
+                       msg = "Error. Please specify a size larger than %llu Mbytes.";
+                       print_error(msg, return_message, (((max_sectors * BYTES_PER_BLOCK) / 1024) / 1024));
+               }
                return -EINVAL;
        }

EDIT - I will push it in my 9.0.0 cleanup branch. Look out for a PR. I will add you as a reviewer.

pkoutoupis commented 1 year ago

The fix is placed in https://github.com/pkoutoupis/rapiddisk/pull/145.

pkoutoupis commented 1 year ago

Closing due to #145 being merged.