From: tytso
Subject: Re: [PATCH] ext4: memory leakage in ext4_mb_init()
Date: Apr 3, 9:53 am 2010

Hi Jing,

If you're wondering why I'm taking a long time to respond to your
patches, it is because they are very ill-formed.  They don't conform
to the submitting patches guidelines, the patch comments don't
adequately explain the why the patch is needed, and what testing has
been done, and you tend to throw in patches that aren't correctly
submitted in the middle of comments, and in some cases it's not clear
whether this patch is suppose to be in addition to the previous patch,
and combined into a separate commit, or kept as two separate patches,
etc.

As a result, it takes, much, MUCH, MUCH longer for me to review the
patches for correctness.  I will eventually get to them, but I may end
up working on other patches which are better formed and easier for me
to evaluate for correctness and quality.

If you do submit new patches, especially in this thread where you have
already submitted so many different patches, I would appreicate it if
you could explicitly state that a particular patch has been superceded
by another, or has been withdrawn.  I will try to keep score on the
patchwork web site (for example, I rejected your bb_free_cache patch),
but you've been so prolific with patches, some of which haven't been
very well explained, that I may have lost track of all of your
submissions.   

Please bear with me.

Best regards,

					- Ted




On Sun, Mar 28, 2010 at 04:13:51PM +0800, jing zhang wrote:
> 2010/3/26, Aneesh Kumar K. V <aneesh.kumar@linux.vnet.ibm.com>:
> > On Tue, 23 Mar 2010 20:47:39 +0800, jing zhang <zj.barak@gmail.com> wrote:
> >> 2010/3/22, tytso@mit.edu <tytso@mit.edu>:
> >> > On Sun, Mar 21, 2010 at 10:01:06PM +0800, jing zhang wrote:
> >> >> From: Jing Zhang <zj.barak@gmail.com>
> >> >>
> >> >> Date: Sun Mar 21 21:59:35     2010
> >> >>
> >> >> Cc: Theodore Ts'o <tytso@mit.edu>
> >> >> Cc: Andreas Dilger <adilger@sun.com>
> >> >> Cc: Dave Kleikamp <shaggy@linux.vnet.ibm.com>
> >> >> Signed-off-by: Jing Zhang <zj.barak@gmail.com>
> >> >>
> >> >> ---
> >> >>
> >> >> --- linux-2.6.32/fs/ext4/mballoc.c	2009-12-03 11:51:22.000000000 +0800
> >> >> +++ ext4_mm_leak/mballoc3.c	2010-03-21 21:37:18.000000000 +0800
> >> >> @@ -2360,6 +2360,24 @@ err_freesgi:
> >> >>  	return -ENOMEM;
> >> >>  }
> >> >>
> >> >> +static void ext4_mb_destroy_backend(struct super_block *sb)
> >> >> +{
> >> >> +	ext4_group_t ngroups = ext4_get_groups_count(sb);
> >> >> +	ext4_group_t i;
> >> >> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> >> >> +	int j;
> >> >> +	int num_meta_group_infos = (ngroups + EXT4_DESC_PER_BLOCK(sb) -1)
> >> >> +					>> EXT4_DESC_PER_BLOCK_BITS(sb);
> >> >> +	for (i = 0; i < ngroups; i++)
> >> >> +		kfree(ext4_get_group_info(sb, i));
> >> >> +
> >> >> +	for (j = 0; j < num_meta_group_infos; j++)
> >> >> +		kfree(sbi->s_group_info[j]);
> >> >> +
> >> >> +	kfree(sbi->s_group_info);
> >> >> +	iput(sbi->s_buddy_cache);
> >> >> +}
> >> >> +
> >> >
> >> > It would be better if this could be done by making ext4_mb_release()
> >> > more flexible, and then calling ext4_mb_release() if there is an error
> >> > setting up the data structures in ext4_mb_init().
> >> >
> >> > 						- Ted
> >> >
> >>
> >> Yeah, Ted, going through ext4_mb_release() is clearer.
> >>
> >> ---
> >>
> >> diff --git a/linux-2.6.32/fs/ext4/mballoc.c b/ext4_mm_leak/mballoc3.c
> >> index bba1282..99ca2de 100644
> >> --- a/linux-2.6.32/fs/ext4/mballoc.c
> >> +++ b/ext4_mm_leak/mballoc3.c
> >> @@ -2417,8 +2417,7 @@ int ext4_mb_init(struct super_block *sb, int
> >> needs_recovery)
> >>
> >>  	sbi->s_locality_groups = alloc_percpu(struct ext4_locality_group);
> >>  	if (sbi->s_locality_groups == NULL) {
> >> -		kfree(sbi->s_mb_offsets);
> >> -		kfree(sbi->s_mb_maxs);
> >> +		ext4_mb_release(sb);
> >
> > We may want to make sure that we can safely call ext4_mb_release that
> > early. what i would suggest is to move s_locality_group allocation
> > before ext4_mb_init. that makes error handling easy
> >
> 
> cool idea, Aneesh, and I got it.
> 
> ---
> 
> --- old-linux-2.6.32/fs/ext4/mballoc.c	2009-12-03 11:51:22.000000000 +0800
> +++ ext4_mm_leak/mballoc3-2.c	2010-03-28 16:13:20.000000000 +0800
> @@ -2397,11 +2397,27 @@ int ext4_mb_init(struct super_block *sb,
>  		i++;
>  	} while (i <= sb->s_blocksize_bits + 1);
> 
> +	sbi->s_locality_groups = alloc_percpu(struct ext4_locality_group);
> +	if (sbi->s_locality_groups == NULL) {
> +		kfree(sbi->s_mb_offsets);
> +		kfree(sbi->s_mb_maxs);
> +		return -ENOMEM;
> +	}
> +	for_each_possible_cpu(i) {
> +		struct ext4_locality_group *lg;
> +		lg = per_cpu_ptr(sbi->s_locality_groups, i);
> +		mutex_init(&lg->lg_mutex);
> +		for (j = 0; j < PREALLOC_TB_SIZE; j++)
> +			INIT_LIST_HEAD(&lg->lg_prealloc_list[j]);
> +		spin_lock_init(&lg->lg_prealloc_lock);
> +	}
> +
>  	/* init file for buddy data */
>  	ret = ext4_mb_init_backend(sb);
>  	if (ret != 0) {
>  		kfree(sbi->s_mb_offsets);
>  		kfree(sbi->s_mb_maxs);
> +		free_percpu(sbi->s_locality_groups);
>  		return ret;
>  	}
> 
> @@ -2415,20 +2431,6 @@ int ext4_mb_init(struct super_block *sb,
>  	sbi->s_mb_order2_reqs = MB_DEFAULT_ORDER2_REQS;
>  	sbi->s_mb_group_prealloc = MB_DEFAULT_GROUP_PREALLOC;
> 
> -	sbi->s_locality_groups = alloc_percpu(struct ext4_locality_group);
> -	if (sbi->s_locality_groups == NULL) {
> -		kfree(sbi->s_mb_offsets);
> -		kfree(sbi->s_mb_maxs);
> -		return -ENOMEM;
> -	}
> -	for_each_possible_cpu(i) {
> -		struct ext4_locality_group *lg;
> -		lg = per_cpu_ptr(sbi->s_locality_groups, i);
> -		mutex_init(&lg->lg_mutex);
> -		for (j = 0; j < PREALLOC_TB_SIZE; j++)
> -			INIT_LIST_HEAD(&lg->lg_prealloc_list[j]);
> -		spin_lock_init(&lg->lg_prealloc_lock);
> -	}
> 
>  	if (sbi->s_proc)
>  		proc_create_data("mb_groups", S_IRUGO, sbi->s_proc,
> 
> 
> >
> >
> >>  		return -ENOMEM;
> >>  	}
> >>  	for_each_possible_cpu(i) {
> >> @@ -2511,7 +2510,8 @@ int ext4_mb_release(struct super_block *sb)
> >>  				atomic_read(&sbi->s_mb_discarded));
> >>  	}
> >>
> >> -	free_percpu(sbi->s_locality_groups);
> >> +	if (sbi->s_locality_groups)
> >> +		free_percpu(sbi->s_locality_groups);
> >>  	if (sbi->s_proc)
> >>  		remove_proc_entry("mb_groups", sbi->s_proc);
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> > -aneesh
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: tytso
Subject: Re: [PATCH] ext4: memory leakage in ext4_mb_init()
Date: Apr 4, 11:08 am 2010

On Sun, Apr 04, 2010 at 09:05:14AM +0800, jing zhang wrote:
> 
> Without the cool git, though I am learning how to take advantage of
> it, I could not manage all the patches delivered. In fact, I dig the
> patches with UltraEdit for modifying the C code, Cygwin for git and
> diff -Npu, and virtual machine for compiling. My kid, 11 years old
> boy, has to share the HP notebook with me playing games.

How much testing are you doing before submitting patches, out of
curiosity?

> I resolve conflicts and dependencies between patches in the way that
> they are carried out by independent guys, since I am told that git is
> cool enough. But indeed I created so many hard work for you, sorry.

Having independent patches is actually better --- but I think you're
misunderstanding what I was complaining about before.  Patches should
that are accepted into mainline should do one and only one thing.  So
if someone suggests that you make changes to your submitted patch,
ideally what you should do is to resubmit the patch with the fixes ---
and not submit a patch which is a delta to the previous one.

This is especially true if the original patch is buggy; one of the
things we try very hard to maintain is that the kernel tree compile
cleanly, and pass the regression test suite, between every single
commit.  In other words, we try to avoid knowingly introducing a
regression in a patch and fixing it in a subsequent patch.  This
allows things like "git bisect" to work, and it also makes it easier
for people to look at the commit history to understand why certain
changes were made, and especially when trying to find how a bug was
introduced into ext4.  Ultimately, this is about keeping the kernel
source code easily maintainable.  This means that incrased code
complexity has to be justified, and code and code changes have to be
meticulously documented.

Best regards,

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


From: jing zhang
Subject: Re: [PATCH] ext4: memory leakage in ext4_mb_init()
Date: Apr 4, 8:53 pm 2010

2010/4/5, tytso@mit.edu <tytso@mit.edu>:
> On Sun, Apr 04, 2010 at 09:05:14AM +0800, jing zhang wrote:
>
> How much testing are you doing before submitting patches, out of
> curiosity?

Yes, Ted, it is curiosity that drives me to do hard works, including patch ext4.
I already told you that I am happy to patch ext4.
I also like to share/deliver my patches to the developers and
maintainers of ext4, simply because I find something not correct or
perfect in the version I got at kernel.org, and because I am not 100%
sure. And review is necessary, as described in Andi Kleen's "On
submitting kernel patches", which I read last night. And I got that on
cmdline,

           git --stat --summary  a/x.c  b/x.c  >  x-01.diff

may get nice output, correct?

It is hard work for me, a newbie, though curious, to do perfect patch,
and I try my best to describe the patch in English, which is not my
native language but one of the most beautiful languages on earth.

What is behind my curiosity is the belief that I will be freed both by
Jesus in West and by Buddha in East, today or tomorrow, if I deliver
what I can to those who need.

And after operations on cmdline, I compile the modified, modprobe, dd,
and rmmod with virtual machine. It is not hard.

> Having independent patches is actually better --- but I think you're
> misunderstanding what I was complaining about before.  Patches should

Whatever you complain, I try to be a good listener every time:)
GNU Linux is free, is MIT free?
And I am free to change, to be responsible for what I did, or maybe
freed by you.

> that are accepted into mainline should do one and only one thing.  So
> if someone suggests that you make changes to your submitted patch,
> ideally what you should do is to resubmit the patch with the fixes ---
> and not submit a patch which is a delta to the previous one.
>
> This is especially true if the original patch is buggy; one of the
> things we try very hard to maintain is that the kernel tree compile
> cleanly, and pass the regression test suite, between every single
> commit.  In other words, we try to avoid knowingly introducing a
> regression in a patch and fixing it in a subsequent patch.  This
> allows things like "git bisect" to work, and it also makes it easier
> for people to look at the commit history to understand why certain
> changes were made, and especially when trying to find how a bug was
> introduced into ext4.  Ultimately, this is about keeping the kernel
> source code easily maintainable.  This means that incrased code
> complexity has to be justified, and code and code changes have to be
> meticulously documented.

I think what is called ext4 will change, and more will be freed by the
change, since it is under your maintenance, at least currently, a
model of maintainer, especially of strict requirement and kind
patience to patches received.

Good weekend, and please review my new patches next week.

Thank you, Ted.

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


From: Eric Sandeen
Subject: Re: [PATCH] ext4: memory leakage in ext4_mb_init()
Date: Apr 4, 9:27 pm 2010

jing zhang wrote:
> 2010/4/5, tytso@mit.edu <tytso@mit.edu>:
>> On Sun, Apr 04, 2010 at 09:05:14AM +0800, jing zhang wrote:
>>
>> How much testing are you doing before submitting patches, out of
>> curiosity?
> 
> Yes, Ted, it is curiosity that drives me to do hard works, including patch ext4.

It is the language barrier that is making some of this difficult,
but I'm not complaining - you speak English much better than I speak any
second language.  :)

Ted meant that -he- was curious about how much testing you were doing.

...

> And after operations on cmdline, I compile the modified, modprobe, dd,
> and rmmod with virtual machine. It is not hard.

More testing than this would be good; dd is very minimal.

One of our new standard tests for et4 is the xfstests test suite from 
http://git.kernel.org/?p=fs/xfs/xfstests-dev.git;a=summary

It is a collection of many tests developed for xfs, but many tests are
generic and can run on ext4 as well.  I would suggest that after you have
several patches ready, you should at least run through the tests in this
collection.  It won't catch every mistake but it runs a large variety of 
tests, much more stressful than dd.

Thanks for your email, and thanks for clearly spending time looking for
ways to improve ext4.  I think that with practice, you will be a good 
contributor.

Ted can certainly be a patient maintainer - read his suggestions and the
kernel patch submission guidelines, and I think you will get better at this.

Do your best to explain the reasons for your patches, and any testing you
have done, and describe any test which can show a bug that you find - 
and we can help to clarify changelogs if they need it.

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


From: jing zhang
Subject: Re: [PATCH] ext4: memory leakage in ext4_mb_init()
Date: Apr 4, 9:51 pm 2010

2010/4/5, Eric Sandeen <sandeen@redhat.com>:
> jing zhang wrote:
>> 2010/4/5, tytso@mit.edu <tytso@mit.edu>:
>>> On Sun, Apr 04, 2010 at 09:05:14AM +0800, jing zhang wrote:
>>>
>>> How much testing are you doing before submitting patches, out of
>>> curiosity?
>>
>> Yes, Ted, it is curiosity that drives me to do hard works, including patch
>> ext4.
>
> It is the language barrier that is making some of this difficult,
> but I'm not complaining - you speak English much better than I speak any

You are good guy:) Is English your native language? And, I am curious,
what is the second language you are able to speak, Eric?

> second language.  :)
>
> Ted meant that -he- was curious about how much testing you were doing.

How do know what Ted meant, by iphone?

>
> ...
>
>> And after operations on cmdline, I compile the modified, modprobe, dd,
>> and rmmod with virtual machine. It is not hard.
>
> More testing than this would be good; dd is very minimal.
>
> One of our new standard tests for et4 is the xfstests test suite from
> http://git.kernel.org/?p=fs/xfs/xfstests-dev.git;a=summary
>
> It is a collection of many tests developed for xfs, but many tests are
> generic and can run on ext4 as well.  I would suggest that after you have
> several patches ready, you should at least run through the tests in this
> collection.  It won't catch every mistake but it runs a large variety of
> tests, much more stressful than dd.

Thanks for your good advice.

>
> Thanks for your email, and thanks for clearly spending time looking for
> ways to improve ext4.  I think that with practice, you will be a good
> contributor.
>
> Ted can certainly be a patient maintainer - read his suggestions and the
> kernel patch submission guidelines, and I think you will get better at this.
>
> Do your best to explain the reasons for your patches, and any testing you
> have done, and describe any test which can show a bug that you find -
> and we can help to clarify changelogs if they need it.
>

I am not good at testing, partially because it is hard to setup the
required environment, sometimes several hard disks are needed, maybe a
few boxes, but I try to analyse the C code while reading and
understanding the works by great maintainers and developers of Linux
kernel.

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


From: Eric Sandeen
Subject: Re: [PATCH] ext4: memory leakage in ext4_mb_init()
Date: Apr 4, 9:59 pm 2010

jing zhang wrote:
> 2010/4/5, Eric Sandeen <sandeen@redhat.com>:
>> jing zhang wrote:
>>> 2010/4/5, tytso@mit.edu <tytso@mit.edu>:
>>>> On Sun, Apr 04, 2010 at 09:05:14AM +0800, jing zhang wrote:
>>>>
>>>> How much testing are you doing before submitting patches, out of
>>>> curiosity?
>>> Yes, Ted, it is curiosity that drives me to do hard works, including patch
>>> ext4.
>> It is the language barrier that is making some of this difficult,
>> but I'm not complaining - you speak English much better than I speak any
> 
> You are good guy:) Is English your native language? And, I am curious,
> what is the second language you are able to speak, Eric?

Only a little German.

>> second language.  :)
>>
>> Ted meant that -he- was curious about how much testing you were doing.
> 
> How do know what Ted meant, by iphone?

No, because I am a native English speaker and I understood the 
figure of speech.  ("out of curiosity")

> I am not good at testing, partially because it is hard to setup the
> required environment, sometimes several hard disks are needed, maybe a
> few boxes, but I try to analyse the C code while reading and
> understanding the works by great maintainers and developers of Linux
> kernel.

Testing really is critical to development; some things can be done by
inspection, but if you don't test it is hard to know if you made a
mistake.

You can always test inside a vm, or on a loopback file, on a single box.

Without testing, you are asking others to do testing for you
(unless the change is so obvious that it can be trusted) 

-Eric
 
>                 - zj


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


From: jing zhang
Subject: Re: [PATCH] ext4: memory leakage in ext4_mb_init()
Date: Apr 4, 10:08 pm 2010

2010/4/5, Eric Sandeen <sandeen@redhat.com>:
> Testing really is critical to development; some things can be done by
> inspection, but if you don't test it is hard to know if you made a
> mistake.
>
> You can always test inside a vm, or on a loopback file, on a single box.
>
> Without testing, you are asking others to do testing for you
> (unless the change is so obvious that it can be trusted)
>

Before linux-2.6.32 is released, would you like tell me, how is ext4 tested?
Is tough testing able to catch all bugs?

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


From: tytso
Subject: Re: [PATCH] ext4: memory leakage in ext4_mb_init()
Date: Apr 5, 5:42 am 2010

On Mon, Apr 05, 2010 at 01:08:58PM +0800, jing zhang wrote:
> 
> Before linux-2.6.32 is released, would you like tell me, how is ext4 tested?
> Is tough testing able to catch all bugs?

For one thing, we use the XFSQA (aka xfstests) test suite.  I usually
run the "quick" set of tests between each commit, and I run a much
longer "auto" test suite before submitting a set of patches to Linus.

For specific patches, we also like to be able to test it specifically.
So often we'll create a small filesystem image and then see what the
file system does with and without the patch.  For example, if we're
worried that a specific file system corruption will cause a BUG_ON,
we'll create a small file system, corrupt it using the debugfs tool,
and then demonstrate that without the patch, we get the BUG_ON.  With
the patch applied, the BUG_ON should go away.

So the patch-specific tests are there so we can make sure the patch
does what we think it should, and it fixes the problem it claims to
fix.  The XFSQA test suite is to there to make sure that we don't
accidentally introduce bugs while trying to improve the file system.

And of course, the final safeguard is patch review by others.  This is
where decent comments and making sure the code is maintainable is
critically important.  Since this takes other developer's time,
especially senior developers like Eric and myself's time, which is
rather valuable and hard to find, it's why patch submitters need
shoulder more of the work in terms of refining patches and
resubmitting new versions of the patches.

						- Ted

P.S.  Yes, Eric was absolutely correct.  I wanted to know how much
testing you had been doing on your patches before submitting them.  At
least few of your patches have caused me to wonder whether the patches
had gone through sufficient testing.
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


From: jing zhang
Subject: Re: [PATCH] ext4: memory leakage in ext4_mb_init()
Date: Apr 6, 6:43 am 2010

2010/4/5, tytso@mit.edu <tytso@mit.edu>:
> On Mon, Apr 05, 2010 at 01:08:58PM +0800, jing zhang wrote:
>>
>> Before linux-2.6.32 is released, would you like tell me, how is ext4
>> tested?
>> Is tough testing able to catch all bugs?
>
> For one thing, we use the XFSQA (aka xfstests) test suite.  I usually
> run the "quick" set of tests between each commit, and I run a much
> longer "auto" test suite before submitting a set of patches to Linus.
>
> For specific patches, we also like to be able to test it specifically.
> So often we'll create a small filesystem image and then see what the
> file system does with and without the patch.  For example, if we're
> worried that a specific file system corruption will cause a BUG_ON,
> we'll create a small file system, corrupt it using the debugfs tool,
> and then demonstrate that without the patch, we get the BUG_ON.  With
> the patch applied, the BUG_ON should go away.
>
> So the patch-specific tests are there so we can make sure the patch
> does what we think it should, and it fixes the problem it claims to
> fix.  The XFSQA test suite is to there to make sure that we don't
> accidentally introduce bugs while trying to improve the file system.
>
> And of course, the final safeguard is patch review by others.  This is


First, Mr. Theodore Ts'o, your reply is a great honor to me.

And I learn more about ext4 and its maintainers and developers,
specially that testing is important to ext4. Thanks.

In my view, one of the important parts of a patch is that the patcher
is really concerning what is patched, another is whether the questions
listed in the patch do exist, and whether the solution, if provided by
the patcher, is correct.

Even if the provide solution is ok, I think, it is the privilege,
responsibility and duty of maintainers to execute the final
submitting, and therefore it is not the duty of patchers to make sure
that the solution is 100% correct and perfect, testing is good or not.

I like to be a maintainer of some subsystem of GNU Linux, since it is
my shame that end users experience panic, crash, bug, bug_on caused by
what is under my maintenance, that end users are treated as rats and
monkeys in laboratory even though I am not paid by any end user, and
the nice reputations of distributors, say red hat, of service
providers, say IBM.

To be a maintainer, I will apologize on my homepage once a patch
received, if the questions listed in the patch do exist, simply
because I abuse my duty and time.


> where decent comments and making sure the code is maintainable is
> critically important.  Since this takes other developer's time,
> especially senior developers like Eric and myself's time, which is

I still concern how to correctly measure the lost, in the next half of
2010, of time and value of end users, say 100, caused by what is buggy
in ext4. Are you sure no buggy, Eric and Ted?

> rather valuable and hard to find, it's why patch submitters need
> shoulder more of the work in terms of refining patches and
> resubmitting new versions of the patches.
>
> 						- Ted
>
> P.S.  Yes, Eric was absolutely correct.  I wanted to know how much

Do you like opera, Mr. Theodore Ts'o?

If you like, I invite you Beijing Opera and tea, and I pay the air
ticket, double trip, from Los Angeles to Beijing. And please info me
your time schedule if you accept my invitation.

> testing you had been doing on your patches before submitting them.  At
> least few of your patches have caused me to wonder whether the patches
> had gone through sufficient testing.
>

If end users of ext4 can not be treated, for any reason, as rats and
monkeys, lets go ahead, please, focusing upon patch.

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


From: tytso
Subject: Re: [PATCH] ext4: memory leakage in ext4_mb_init()
Date: Apr 6, 7:21 am 2010

On Tue, Apr 06, 2010 at 09:43:58PM +0800, jing zhang wrote:
> 
> In my view, one of the important parts of a patch is that the patcher
> is really concerning what is patched, another is whether the questions
> listed in the patch do exist, and whether the solution, if provided by
> the patcher, is correct.
> 
> Even if the provide solution is ok, I think, it is the privilege,
> responsibility and duty of maintainers to execute the final
> submitting, and therefore it is not the duty of patchers to make sure
> that the solution is 100% correct and perfect, testing is good or not.

Well, the problem is there are many more patch submitters than there
are maintainers, and so your proposal simply doesn't scale.  Consider
that for some maintainers, there may be 10 or 20 or 30 or more patch
submitters in their subsystem.  With that kind of
submitter-to-maintainer ratio, the patch submitter simply has to do
much more of the work, since otherwise the subsystem maintainer simply
can't keep up.

Most maintainers actually spend much less time dealing with patch
submitters than I have invested with you.  They simply send a NACK,
and maybe 2-3 sentences explaning of what still needs fixing, and then
it's up to the patch submitter to resubmit the patch.  Some patches
are resubmitted 5, 6 times before the maintainer finally accepts it.

I happen to believe that we need to encourage newcomers to the kernel
developer community, and so I spend more time mentoring people who are
new to the process.  But at the end of the day, I have only so many
hours that I can spend on newcomers, and so after a while, I will
start serving other patch submitters who are more capable of providing
patches that are easy to review and integrate.

> I like to be a maintainer of some subsystem of GNU Linux, since it is
> my shame that end users experience panic, crash, bug, bug_on caused by
> what is under my maintenance, that end users are treated as rats and
> monkeys in laboratory even though I am not paid by any end user, and
> the nice reputations of distributors, say red hat, of service
> providers, say IBM.

Sure, but that means the patches which are submitted has to be high
quality.  If I get half-tested patches, or patches where you can't
give me a reproduction case that demonstrates the BUG_ON, and it's not
obvious whether or not the patch is safe, I have to worry about the
possibility that applying your patch may make the the file system more
likely to crash, not less.

Ext4 is actually quite stable at this point.  Very large numbers of
people are using it, and most users are quite happy.  So at this
point, I have to weigh the risk that a patch might introduce a bug
against the claim that it fixes the bug --- especially if the patch
submitter can't explain how the BUG_ON might have been triggered
without their patch, and can't explain to me how much testing he or
she has done before sumitting the patch to me.

As far as your concern about end users getting treated as "rats and
monkeys", they are certainly getting paid; they get to use very high
quality software for free!  The tradeoff is that they won't get as
much support compared to someone who is a paying customer of Red Hat
or IBM.  For the right customer, when I worked at IBM, I would fly on
site to a bank in New York City and fix their bug.  And I'm sure it's
the same for Eric, if someone has a critical bug and they are a big
Red Hat customer.  Those who pay our salaries, get the best support.
End users who aren't paying support to a big Linux company still get
support, but it won't be as good, and that's just life.  End users who
want to use the latest code, are in fact guinea pigs.  If they don't
want to guinea pigs, they can use Fedora kernels or OpenSuSE kernels.

> > where decent comments and making sure the code is maintainable is
> > critically important.  Since this takes other developer's time,
> > especially senior developers like Eric and myself's time, which is
> 
> I still concern how to correctly measure the lost, in the next half of
> 2010, of time and value of end users, say 100, caused by what is buggy
> in ext4. Are you sure no buggy, Eric and Ted?

There is no such thing as code which is not buggy.  For any
non-trivial program, it's almost certain there are bugs.  The only
question is how easy it is for the someone to trigger the bugs, and
what are the consequences if the bugs get triggered.  During ext4
development, we've found bugs that were around in for over a decade in
ext3, but it was simply something that no one had ever tripped over.
(It required a certain race condition getting hit, plus a power
failure or other unclean shutdown very shortly after the race
condition, and it was simply something which no one had ever noticed,
or if they did trip over it, they assumed it was caused by a hardware
problem.)

Ext4 is not exempt from these fundamental laws of software
engienering.  "Code is always buggy until the last user of the program
dies".

As far as the time value of users, remember that people who get free
code, get what they pay for.  We all have our salaries paid by
someone, and at the end of the day, our priorities are driven by our
employers.  So I will track down a bug report from an end user because
(a) that bug might affect the machines that I am paid to support, and
(b) a larger ext4 user base is good for the community in general, and
there are secondary benefits to my employer that accrue from that, and
(c) Google is just a great company.  :-)    

(As is Red Hat, SuSE, Oracle, etc. :-)

But please don't get fooled that the resources we have to work on
problems from end users, or time that I can spend mentoring newcomers
to the kernel developer community, is infinite.  Because it isn't.
I'll spend some of my own time, and work late at nights, to help end
users who are getting linux "for free", especially if they run into
problems with ext4, because I like to help users as much as I can.
And I will help out newcomers like you because it's personally
important to me.  But I do need to sleep, and I do have other
priorities, like family and friends.  And at the end of the day,
because I like food with my meals, the needs of my $DAYJOB, are going
to get priority, at least during work hours.

Finally, keep in mind that the maxim that code which is not buggy also
applies to your patches.  At least some of your patches are definitely
buggy, and which brings us back to the question whether things will be
made better or worse if we were to apply all of your patches.  And in
the meantime, there are patches from other patch submitters which are
proven themselves to be much more likely to be correct, and easier to
review and integrate.  

If you were a maintainer, faced with limited time and resources, and
someone who floods you with a large number of patches that take extra
time to review and comprehend, and that person refuses to help you
make life easier by reworking their patches so that they are easier to
review and comprehend, what would _you_ do?

Best regards,

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