Re: How do I do it? (was Re: Accountability)

Benjamin LaHaise (kernel@kvack.org)
Fri, 17 Sep 1999 11:46:16 -0400 (EDT)


On Thu, 16 Sep 1999, Andreas Dilger wrote:

> On a more positive side of the "Accountability" thread, let's see what
> CAN be done to get things into the kernel... I'm a relative newcomer
> here (too much traffic for my liking before I had a kernel patch to care
> about), so bear with me.
>
> I have written a kernel patch which will allow resizing of an ext2
> filesystem while it is mounted (online-ext2 is available at my web page:
> http://www-mddsp.enel.ucalgary.ca/People/adilger/online-ext2/
>
> 1) My kernel patch does the minimal work it needs to, and the rest is done
> as user-space utilities, which I think is the desirable way to go, as
> we don't wan't the kernel to be able to do a mke2fs necessarily. So far
> so good?

(without having looked at the code) Code-wise, that sounds reasonable.
Since you're talking about patches to a specific subsystem, it would be
better if you send a note directly to the maintainer and people who tend
to work on the code in addition to the linux-kernel mailing list. Also,
given the volume of linux-kernel, it is much better to make intelligent
use of the subject line, such as the [PATCH], [RFC], and [RFT] (request
for testing) prefixes that keep cropping up.

> 2) I re-used what existing kernel code I could, made it more generic in
> some cases, to handle what I needed to do. Presumably this is the
> correct way to go?

Everyone should code like that! =)

> 3) I have read about formatting of code for the kernel, and have tried to
> follow them. Would patches be rejected for having too many comments?

Unlikely. Patches are usually rejected because the code is unclear, not
because it is over-documented. Put slightly differently: if the comment
increases the readability of the code (usually for subtle points), then
it's a good idea, but if the code can't be understood without the
comments, then something's probably wrong with the code. Please note that
this is my understanding of things, which I'm sure will be corrected if
anyone thinks I'm off base. (I get the feeling I'm paraphrasing someone
here.)

> 4) There are some known "issues" in the patch (e.g. it turns off quota
> checking while not having the lock on the filesystem for a "brief instant"
> because it is calling a function which grabs the superblock lock itself).
> Is this "permitted", considering the combination of quotas, frequency of
> filesystem resize, and a "one-line" chance of being interrupted before
> getting the superblock lock again is considered small enough to justify
> not making more changes to the kernel?

This is where the 'making noise' stage is quite useful for soliciting
input. However, for bugs which are known (and small), it's better to fix
them before finalizing the patch.

> 5) Since the userspace tools aren't 100% ready for public consumption, is
> there any chance of getting the patch into 2.3.x? I wanted to have the
> user code ready before trying to get the patch into the mainstream kernel
> (to avoid more patches to the kernel), but in my testing the kernel code
> is ready for use. The resize functions added by the patch sits totally off
> the main code path, so do not have any real performance impact at all if
> they are added.

Frequently, new features that appear in the kernel have only the tools
that the developer has used to test them. By making sure that people know
about what you've implemented, you increase the chance that someone else
will help build upon them.

> 6) With a new feature like this, would it be required to have an option to
> turn it on/off via menuconfig? The patch will add a useful capability
> to the kernel even if people don't have the user-space tools required for
> full functionality (ie an unmodified "mount" command can do wonders for
> you if required). However, to really get use out of this patch, you need
> to be able to resize your underlying device size via md or hardware RAID,
> or most likely LVM

I've just gone over the patch, and in my opinion, it is sufficiently small
that making it a config option seems like overkill. It touches very
little code, and the new feature doesn't affect normal use. Way cool!

-ben

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/