Preparing btier for kernel inclusion

Preparing btier for kernel inclusion

[Mark Ruijter]

Hi all,

Btier is a kernel based block device that supports automatic policy 
based data migration.
It allows you to assemble a virtual block device from other block 
devices of even files.

For example you can assemble a btier block device that consists of : 
/dev/ssd:/dev/LSIsasArray:/mnt/myNetApp/nfs-disk03.img
Since btier uses the ssd as part of the whole device and not merely as 
cache the capacity of the SSD is a part of the total capacity.
Btier will differentiate between random and sequential IO. And will 
therefore redirect random IO to the most suitable device.
Which in this case would be the SSD.

Another trick that btier uses to speed up random IO is to write it 
sequentially.

Btier performance compared to SSD cache projects (tested with fio, 
details on lessfs.com)
grep iops bcache.txt
Jobs: 1 (f=1): [___w] [87.4% done] [0K/142.3M /s] [0 /35.6K iops] [eta 
00m:34s]
   read : io\x12288MB, bw"8324KB/s, iopsW080 , runt= 55110msec
   read : io\x11250MB, bw\x191989KB/s, iopsG997 , runt= 60001msec
   write: ios94.5MB, bw\x126195KB/s, iops1548 , runt= 60002msec
   write: ioy24.2MB, bw\x135237KB/s, iops3809 , runt= 60001msec
root@ctrl01:/opt/fio# grep iops eio.txt
   read : io\x12288MB, bw%4782KB/s, iopsc695 , runt= 49387msec
   read : ioI42.9MB, bw„356KB/s, iops!089 , runt= 60001msec
   write: io\x11134MB, bw\x190018KB/s, iopsG504 , runt= 60001msec
   write: iow80.8MB, bw\x132788KB/s, iops3197 , runt= 60001msec
root@ctrl01:/opt/fio# grep iops btier.txt
   read : io\x12288MB, bwD8333KB/s, iops\x112083 , runt= 28066msec
   read : io$39.6MB, bwA635KB/s, iops\x10408 , runt= 60001msec
   write: io\x12288MB, bwI3041KB/s, iops\x123260 , runt= 25521msec
   write: io‘72.2MB, bw\x156550KB/s, iops9137 , runt= 60001msec

Although performance is always important, btier is _not_ an SSD cache 
project.
Instead it is much more about ILM.

Before considering to submit btier for inclusion it would be nice if the 
code is looked at from a janitors perspective.
You can find the code at : http://sourceforge.net/projects/tier/files/ 
and general information at : http://www.lessfs.com

I look forward to hearing from you.
Thanks in advance for any feedback that you would like to share.

Mark Ruijter
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Preparing btier for kernel inclusion

[Dan Carpenter]

This is staging quality code.  (Not very good).  The staging tree is
closed for the 3.9 window so it couldn't make it in until 3.10.

*) Clean up the indenting.
*) Run checkpatch.pl over this and fix the warnings.
*) Don't include .c files into other .c files.
*) Get rid of homemade print macros like TIERERR().  Use dev_err()
   or pr_err().
*) Delete all compat code with older kernels like
	#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,0,0)
*) Change Yoda code to the if (NULL = odinfo) to normal format
	if (!odinfo)
		return -ENOMEM;
   Or "if (NULL != dev->backdev[0]->blocklist)" should be:
	if (dev->backdev[0]->blocklist)
		memcpy( ...
*) Run Sparse and Smatch over the code.  This should complain about
   some poor error handling.  For example, printing "no memory"
   followed by a dereference instead of actual error handling.  Or
   in the ioctl() it returns with the lock held.

*) Make magic numbers a define.
	/* Allow max 24 devices to be configured */ 
	devicenames = kmalloc(sizeof(char) * 26, GFP_KERNEL);

	for (count = 0; count <= 25; count++) {
		devicenames[count] = 97 + count;
	}

   Where does the 97 come from?  Also it's hella messy to use
   24, 25, and 26!  The for loop should be written in the normal
   way:

	#define MAX_BTEIR_DEVS 26

	for (i = 0; i < MAX_BTEIR_DEVS; i++) {

   Btw, use "i" instead of "count" for the iterator.  Use "count"
   for counting.

   Some of the magic numbers are just wrong:
	res = snprintf(buf, 1023, "%s\n", msg);
   1023 should have been PAGE_SIZE.  If the size argument to
   snprintf() is non-zero then snprintf() adds a NUL terminator.
   No one ever creates a 1023 char buffer.  Also the return value
   is the number of bytes that would have been printed if there
   were enough space (not counting the NUL terminator).  Consider
   using scnprintf().

*) Use normal kernel style comments.
	/* single line comment */

	/*
	 * Multi line
	 * comment.
	 */

*) Some of functions could use more comments.  What does
   allocated_on_device() return?  I would have assumed from the name
   that it returns a bool, but actually it returns a u64.

*) It scares me that when list_for_each_safe() is used
   unnecessarily.  A lot of people assume it has to do with locking
   but it doesn't.  It's for when you remove a list item.  This is
   wrong:

	list_for_each_safe(pos, q, &device_list) {
		count++;
	}

*) Put a blank line between declarations and code.

*) Use temp variables to make lines shorter:

-	dev->backdev[count]->bitlistsize -		dev->backdev[count]->devmagic->bitlistsize;

+	back = dev->backdev[count];
+	back->bitlistsize = back->devmagic->bitlistsize;

*) Never return -1 as a error code.  Return proper error codes at
   every level.

*) The TIER_DEREGISTER ioctl takes a kernel pointer from user space
   which is a bug.  It should be doing copy_from_user().

There are a lot of other messy things about this code, but that
should be enough to get started.

My advice is that people will take you a lot more seriously if you
clean it up and make a good first impression.  The block layer
people are crotchety.

Also, when you send patches for review, send it as a patch which can
be reviewed without leaving the email client.  That way we can put
comments inline.

regards,
dan carpenter

Re: Preparing btier for kernel inclusion

[Mark Ruijter]

Hi Dan,

Thanks for your elaborate and fast response.
I will change things as suggested and get back afterwards.

On quick question about indenting:
Is there a set of options to indent that can produce indented code that 
is acceptable?
Like : indent -npro -kr -i8 -ts8 -sob -l80 -ss -ncs -cp1 *.c

Or is manual labour a requirement? ;-)

Best regards,

Mark

--
On 02/12/2013 10:47 AM, Dan Carpenter wrote:
> This is staging quality code.  (Not very good).  The staging tree is
> closed for the 3.9 window so it couldn't make it in until 3.10.
>
> *) Clean up the indenting.
> *) Run checkpatch.pl over this and fix the warnings.
> *) Don't include .c files into other .c files.
> *) Get rid of homemade print macros like TIERERR().  Use dev_err()
>     or pr_err().
> *) Delete all compat code with older kernels like
> 	#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,0,0)
> *) Change Yoda code to the if (NULL = odinfo) to normal format
> 	if (!odinfo)
> 		return -ENOMEM;
>     Or "if (NULL != dev->backdev[0]->blocklist)" should be:
> 	if (dev->backdev[0]->blocklist)
> 		memcpy( ...
> *) Run Sparse and Smatch over the code.  This should complain about
>     some poor error handling.  For example, printing "no memory"
>     followed by a dereference instead of actual error handling.  Or
>     in the ioctl() it returns with the lock held.
>
> *) Make magic numbers a define.
> 	/* Allow max 24 devices to be configured */
> 	devicenames = kmalloc(sizeof(char) * 26, GFP_KERNEL);
>
> 	for (count = 0; count <= 25; count++) {
> 		devicenames[count] = 97 + count;
> 	}
>
>     Where does the 97 come from?  Also it's hella messy to use
>     24, 25, and 26!  The for loop should be written in the normal
>     way:
>
> 	#define MAX_BTEIR_DEVS 26
>
> 	for (i = 0; i < MAX_BTEIR_DEVS; i++) {
>
>     Btw, use "i" instead of "count" for the iterator.  Use "count"
>     for counting.
>
>     Some of the magic numbers are just wrong:
> 	res = snprintf(buf, 1023, "%s\n", msg);
>     1023 should have been PAGE_SIZE.  If the size argument to
>     snprintf() is non-zero then snprintf() adds a NUL terminator.
>     No one ever creates a 1023 char buffer.  Also the return value
>     is the number of bytes that would have been printed if there
>     were enough space (not counting the NUL terminator).  Consider
>     using scnprintf().
>
> *) Use normal kernel style comments.
> 	/* single line comment */
>
> 	/*
> 	 * Multi line
> 	 * comment.
> 	 */
>
> *) Some of functions could use more comments.  What does
>     allocated_on_device() return?  I would have assumed from the name
>     that it returns a bool, but actually it returns a u64.
>
> *) It scares me that when list_for_each_safe() is used
>     unnecessarily.  A lot of people assume it has to do with locking
>     but it doesn't.  It's for when you remove a list item.  This is
>     wrong:
>
> 	list_for_each_safe(pos, q, &device_list) {
> 		count++;
> 	}
>
> *) Put a blank line between declarations and code.
>
> *) Use temp variables to make lines shorter:
>
> -	dev->backdev[count]->bitlistsize > -		dev->backdev[count]->devmagic->bitlistsize;
>
> +	back = dev->backdev[count];
> +	back->bitlistsize = back->devmagic->bitlistsize;
>
> *) Never return -1 as a error code.  Return proper error codes at
>     every level.
>
> *) The TIER_DEREGISTER ioctl takes a kernel pointer from user space
>     which is a bug.  It should be doing copy_from_user().
>
> There are a lot of other messy things about this code, but that
> should be enough to get started.
>
> My advice is that people will take you a lot more seriously if you
> clean it up and make a good first impression.  The block layer
> people are crotchety.
>
> Also, when you send patches for review, send it as a patch which can
> be reviewed without leaving the email client.  That way we can put
> comments inline.
>
> regards,
> dan carpenter
>

Re: Preparing btier for kernel inclusion

[walter harms]

IMHO,
there is a indent line in the kernel styleguide.

re,
 wh


Am 12.02.2013 11:09, schrieb Mark Ruijter:
> 
> Hi Dan,
> 
> Thanks for your elaborate and fast response.
> I will change things as suggested and get back afterwards.
> 
> On quick question about indenting:
> Is there a set of options to indent that can produce indented code that
> is acceptable?
> Like : indent -npro -kr -i8 -ts8 -sob -l80 -ss -ncs -cp1 *.c
> 
> Or is manual labour a requirement? ;-)
> 
> Best regards,
> 
> Mark
> 
> -- 
> On 02/12/2013 10:47 AM, Dan Carpenter wrote:
>> This is staging quality code.  (Not very good).  The staging tree is
>> closed for the 3.9 window so it couldn't make it in until 3.10.
>>
>> *) Clean up the indenting.
>> *) Run checkpatch.pl over this and fix the warnings.
>> *) Don't include .c files into other .c files.
>> *) Get rid of homemade print macros like TIERERR().  Use dev_err()
>>     or pr_err().
>> *) Delete all compat code with older kernels like
>>     #if LINUX_VERSION_CODE >= KERNEL_VERSION(3,0,0)
>> *) Change Yoda code to the if (NULL = odinfo) to normal format
>>     if (!odinfo)
>>         return -ENOMEM;
>>     Or "if (NULL != dev->backdev[0]->blocklist)" should be:
>>     if (dev->backdev[0]->blocklist)
>>         memcpy( ...
>> *) Run Sparse and Smatch over the code.  This should complain about
>>     some poor error handling.  For example, printing "no memory"
>>     followed by a dereference instead of actual error handling.  Or
>>     in the ioctl() it returns with the lock held.
>>
>> *) Make magic numbers a define.
>>     /* Allow max 24 devices to be configured */
>>     devicenames = kmalloc(sizeof(char) * 26, GFP_KERNEL);
>>
>>     for (count = 0; count <= 25; count++) {
>>         devicenames[count] = 97 + count;
>>     }
>>
>>     Where does the 97 come from?  Also it's hella messy to use
>>     24, 25, and 26!  The for loop should be written in the normal
>>     way:
>>
>>     #define MAX_BTEIR_DEVS 26
>>
>>     for (i = 0; i < MAX_BTEIR_DEVS; i++) {
>>
>>     Btw, use "i" instead of "count" for the iterator.  Use "count"
>>     for counting.
>>
>>     Some of the magic numbers are just wrong:
>>     res = snprintf(buf, 1023, "%s\n", msg);
>>     1023 should have been PAGE_SIZE.  If the size argument to
>>     snprintf() is non-zero then snprintf() adds a NUL terminator.
>>     No one ever creates a 1023 char buffer.  Also the return value
>>     is the number of bytes that would have been printed if there
>>     were enough space (not counting the NUL terminator).  Consider
>>     using scnprintf().
>>
>> *) Use normal kernel style comments.
>>     /* single line comment */
>>
>>     /*
>>      * Multi line
>>      * comment.
>>      */
>>
>> *) Some of functions could use more comments.  What does
>>     allocated_on_device() return?  I would have assumed from the name
>>     that it returns a bool, but actually it returns a u64.
>>
>> *) It scares me that when list_for_each_safe() is used
>>     unnecessarily.  A lot of people assume it has to do with locking
>>     but it doesn't.  It's for when you remove a list item.  This is
>>     wrong:
>>
>>     list_for_each_safe(pos, q, &device_list) {
>>         count++;
>>     }
>>
>> *) Put a blank line between declarations and code.
>>
>> *) Use temp variables to make lines shorter:
>>
>> -    dev->backdev[count]->bitlistsize >> -        dev->backdev[count]->devmagic->bitlistsize;
>>
>> +    back = dev->backdev[count];
>> +    back->bitlistsize = back->devmagic->bitlistsize;
>>
>> *) Never return -1 as a error code.  Return proper error codes at
>>     every level.
>>
>> *) The TIER_DEREGISTER ioctl takes a kernel pointer from user space
>>     which is a bug.  It should be doing copy_from_user().
>>
>> There are a lot of other messy things about this code, but that
>> should be enough to get started.
>>
>> My advice is that people will take you a lot more seriously if you
>> clean it up and make a good first impression.  The block layer
>> people are crotchety.
>>
>> Also, when you send patches for review, send it as a patch which can
>> be reviewed without leaving the email client.  That way we can put
>> comments inline.
>>
>> regards,
>> dan carpenter
>>
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe
> kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: Preparing btier for kernel inclusion

[Dan Carpenter]

On Tue, Feb 12, 2013 at 11:09:00AM +0100, Mark Ruijter wrote:
> 
> Hi Dan,
> 
> Thanks for your elaborate and fast response.
> I will change things as suggested and get back afterwards.
> 
> On quick question about indenting:
> Is there a set of options to indent that can produce indented code
> that is acceptable?
> Like : indent -npro -kr -i8 -ts8 -sob -l80 -ss -ncs -cp1 *.c
> 
> Or is manual labour a requirement? ;-)

Don't look for shortcuts.  Just go through it manually and rewrite
every function slightly.

Starting from the top with tier_sysfs_init().  Change it to:

static int tier_sysfs_init(struct tier_device *dev)
{
	return sysfs_create_group(&disk_to_dev(dev->gd)->kobj,
				  &tier_attribute_group);
}

Next function read_device_magic():

1) Use sizeof(*dmagic) instead of sizeof(struct devicemagic).  This
   means we don't have to look up how "struct devicemagic" and
   *dmagic are related.
2) There is no error handling for the kzalloc().
3) Space after comma in "dev,count".
4) The "(char *)dmagic" cast is ugly.  tier_file_read() should take
   a void pointer instead of a char pointer.
5) Use sizeof(*dmagic) instead of sizoef(struct devicemagic) again.
6) The call to tier_file_read() is less than 80 characters long so
   it can fit on one line.
7) Why does the call to tier_file_read() not have error handling?
8) tier_file_read() should take a pointer to dev->backdev[count]
   instead of count.  It's ugly how count is passed to the lowest
   levels.

Just do it slowly and methodically.  There is about 40 hours of
cleanup here before it can go for review to the block dev people.
If that seems like a lot, then you might want to ask the block
layer people to review the idea and see if it's worth investing the
time to tidy up the code.

Btw, my thinking is that after cleanup you should send this directly
to the block dev people instead of trying to merge it through
staging.  When it's in staging then it's mostly Greg KH and I who
review those patches.  I'm not able to review block layer code
properly and that's where the more important and difficult review
will happen.

regards,
dan carpenter