• Immutable Page
  • Info
  • Attachments

Diff for "OPWBtrfsCleanupTasks"

Differences between revisions 2 and 3

Deletions are marked like this. Additions are marked like this.
Line 1: Line 1:
== Remove redundant local zero structure ==

`fs/btrfs/ioctl.c:btrfs_is_empty_uuid()` compares the callers uuid argument with a static array of zeros. That array should be removed. The memcmp() can compare the caller's uuid with the kernel's global `ZERO_PAGE`.

Taking it further: make a kernel-wide function that tests whether a given buffer only contains zeros. There's no reason to bring a buffer of zeros in to the cache for this. (Maybe such a thing already exists somewhere?)

== pack btrfs structures ==

`pahole` is a program that describes the physical layout of C structures. Some btrfs structures have unused internal padding because the elements are ordered to create holes. Run {{{ $ pahole -y btrfs_ fs/btrfs/btrfs.o}}} and look for the XXX comments in the resulting annotated structure definitions to find the holes. We can discuss possible fixes as needed.
This page lists some possible cleanups that I've seen while reading btrfs code. Instances of these possible cleanups might come and go over time as patches are applied to address them and as new code adds new instances of the problems.
Line 13: Line 5:
btrfs contains many instances of the code pattern: Sometimes people issue a warning based on a condition by writing:
Line 20: Line 12:
This is silly because `WARN_ON()` returns its condition argument specifically so that people can use: This is more work than is necessary because `WARN_ON()` returns its condition argument specifically so that people can write:
Line 26: Line 18:
Many many instances of this can be found with grep. For example: Instances of this can be found with grep. For example:
Line 34: Line 26:
== Use `||` in ref_for_same_block == == Replacing multiple atomic_inc or _dec with _add or _sub ==
Line 36: Line 28:
`fs/btrfs/backref.c:ref_for_same_block()` contains code like:

{{{if (a)
  return 0;
if (b)
  return 0;
if (c)
  return 0;}}}

And so on. For 6 conditions. It should be rewritten as
Sometimes people write multiple atomic increments immediately following each other, like:
Line 48: Line 31:
if (a || b || c)
  return 0;
  atomic_inc(&something);
  atomic_inc(&something);
Line 51: Line 34:

This can be done with both less source code and fewer compiled instructions with:

{{{
  atomic_add(&something, 2);
}}}

The same applies to `atomic_dec()` and `atomic_sub()`

== replace kmalloc(size * nr, ) with kmalloc_array(nr, size) ==

Instances of the pattern:

{{{
  kmalloc(sizeof(something) * nr, ...)
}}}

should be replaced with

{{{
  kmalloc_array(nr, sizeof(something), ...)
}}}

`kmalloc_array()` is preferred because it can check that the calculation doesn't wrap and won't return a smaller allocation.

== add helper for free_root_pointers() ==

`disk-io.c:free_root_pointers()` contains many copies of the following code:

{{{
        if (info->dev_root) {
                free_extent_buffer(info->dev_root->node);
                free_extent_buffer(info->dev_root->commit_root);
                info->dev_root->node = NULL;
                info->dev_root->commit_root = NULL;
        }
}}}

For each root pointer. A helper function should be created which is called for each of the root pointers.

== remove useless variables ==

Sometimes tiny functions add a useless variable for returning the result of another function. The pattern is:

{{{
int function(arguments)
{
    int ret;
    ret = another_function(arguments);
    return ret;
}
}}}

This serves no purpose. At the very least, the function should just return the result of the called function:

{{{
int function(arguments)
{
    return another_function(arguments);
}
}}}

This page lists some possible cleanups that I've seen while reading btrfs code. Instances of these possible cleanups might come and go over time as patches are applied to address them and as new code adds new instances of the problems.

Use WARN_ON()s return value

Sometimes people issue a warning based on a condition by writing:

if (condition) {
    WARN_ON(1);

This is more work than is necessary because WARN_ON() returns its condition argument specifically so that people can write:

  if (WARN_ON(condition)) {

It results in clearer source code and the resulting kernel warning message will output the source code line that contains the condition, not just the WARN_ON(1) call.

Instances of this can be found with grep. For example:

 $ grep -B 1 'WARN_ON(1)' fs/btrfs/*.c | head
fs/btrfs/backref.c-             if (!level) {
fs/btrfs/backref.c:                     WARN_ON(1);

Replacing multiple atomic_inc or _dec with _add or _sub

Sometimes people write multiple atomic increments immediately following each other, like:

  atomic_inc(&something);
  atomic_inc(&something);

This can be done with both less source code and fewer compiled instructions with:

  atomic_add(&something, 2);

The same applies to atomic_dec() and atomic_sub()

replace kmalloc(size * nr, ) with kmalloc_array(nr, size)

Instances of the pattern:

  kmalloc(sizeof(something) * nr, ...)

should be replaced with

  kmalloc_array(nr, sizeof(something), ...)

kmalloc_array() is preferred because it can check that the calculation doesn't wrap and won't return a smaller allocation.

add helper for free_root_pointers()

disk-io.c:free_root_pointers() contains many copies of the following code:

        if (info->dev_root) {
                free_extent_buffer(info->dev_root->node);
                free_extent_buffer(info->dev_root->commit_root);
                info->dev_root->node = NULL;
                info->dev_root->commit_root = NULL;
        }

For each root pointer. A helper function should be created which is called for each of the root pointers.

remove useless variables

Sometimes tiny functions add a useless variable for returning the result of another function. The pattern is:

int function(arguments)
{
    int ret;
    ret = another_function(arguments);
    return ret;
}

This serves no purpose. At the very least, the function should just return the result of the called function:

int function(arguments)
{
    return another_function(arguments);
}

Implement a core btrfs 'find item' interface

This one is more advanced, but I thought I'd throw it out there.

There are many btrfs functions that manually search the tree for an item. They all reimplement the same mechanism and differ in the conditions that they use to find the item.

__inode_info() is one such example. It wants to set the caller's path to point to the first item in the tree after its specified objectid, type, and offset for which objectid and type match the input.

So a core function should be written that does this and __inode_info() should call it.

From there we can start identifying other callers that could use the core item searching function.

Tell others about this page:

last edited 2013-10-30 19:31:49 by ZachBrown