Jens Axboe wrote:
> On Fri, May 29 2009, Artem Bityutskiy wrote:
>> Jens Axboe wrote:
>>>> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
>>>> index 2349e2c..d1ac967 100644
>>>> --- a/fs/ubifs/super.c
>>>> +++ b/fs/ubifs/super.c
>>>> @@ -1929,6 +1929,9 @@ static int ubifs_fill_super(struct super_block *sb, void *data, int silent)
>>>> err = bdi_init(&c->bdi);
>>>> if (err)
>>>> goto out_close;
>>>> + err = bdi_register(&c->bdi, NULL, "ubifs");
>>>> + if (err)
>>>> + goto out_close;
>>> Not quite right, you need to call bdi_destroy() if you have done the
>>> init.
>> Right, bdi_destroy() is already there for long time.
>> I'm confused.
>>
>>> I committed this one this morning:
>>>
>>> http://git.kernel.dk/?p=linux-2.6-block.git;a=commit;h=570a2fe1df85741988ad0ca22aa406744436e281
>> Hmm, it is the same as my patch, but you do
>> + err = bdi_register(&c->bdi);
>> while I do
>> + err = bdi_register(&c->bdi, NULL, "ubifs");
>
> Oops, that's my bad. If you combine the two, we should have a working
> patch :-)
>
>>> But feel free to commit/submit to the ubifs tree directly, then it'll
>>> disappear from my tree once it is merged.
>> Yeah, I think it can go via my tree. I'd merge it at
>> 2.6.31 window. This change does not depend on your
>> work anyway.
>
> Right, I'll just carry the fixup patches meanwhile as well, but wont
> upstream them.
Just to make sure I understood you correctly. I assume my original
patch is fine (because there is bdi_destroy()) and merge it to
ubifs tree.
--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
On Wed, Jun 03 2009, Artem Bityutskiy wrote:
> Jens Axboe wrote:
>> On Fri, May 29 2009, Artem Bityutskiy wrote:
>>> Jens Axboe wrote:
>>>>> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
>>>>> index 2349e2c..d1ac967 100644
>>>>> --- a/fs/ubifs/super.c
>>>>> +++ b/fs/ubifs/super.c
>>>>> @@ -1929,6 +1929,9 @@ static int ubifs_fill_super(struct super_block *sb, void *data, int silent)
>>>>> err = bdi_init(&c->bdi);
>>>>> if (err)
>>>>> goto out_close;
>>>>> + err = bdi_register(&c->bdi, NULL, "ubifs");
>>>>> + if (err)
>>>>> + goto out_close;
>>>> Not quite right, you need to call bdi_destroy() if you have done the
>>>> init.
>>> Right, bdi_destroy() is already there for long time.
>>> I'm confused.
>>>
>>>> I committed this one this morning:
>>>>
>>>> http://git.kernel.dk/?p=linux-2.6-block.git;a=commit;h=570a2fe1df85741988ad0ca22aa406744436e281
>>> Hmm, it is the same as my patch, but you do
>>> + err = bdi_register(&c->bdi);
>>> while I do
>>> + err = bdi_register(&c->bdi, NULL, "ubifs");
>>
>> Oops, that's my bad. If you combine the two, we should have a working
>> patch :-)
>>
>>>> But feel free to commit/submit to the ubifs tree directly, then it'll
>>>> disappear from my tree once it is merged.
>>> Yeah, I think it can go via my tree. I'd merge it at
>>> 2.6.31 window. This change does not depend on your
>>> work anyway.
>>
>> Right, I'll just carry the fixup patches meanwhile as well, but wont
>> upstream them.
>
> Just to make sure I understood you correctly. I assume my original
> patch is fine (because there is bdi_destroy()) and merge it to
> ubifs tree.
It needs to be:
err = bdi_register(&c->bdi, NULL, "ubifs");
if (err)
goto out_bdi;
so you hit the bdi_destroy() for that failure, not goto out_close;
Otherwise it was fine.
--
Jens Axboe
Jens Axboe wrote:
>> Just to make sure I understood you correctly. I assume my original
>> patch is fine (because there is bdi_destroy()) and merge it to
>> ubifs tree.
>
> It needs to be:
>
> err = bdi_register(&c->bdi, NULL, "ubifs");
> if (err)
> goto out_bdi;
>
> so you hit the bdi_destroy() for that failure, not goto out_close;
> Otherwise it was fine.
Ah, I see. Rather non-typical convention though. I expected
bdi_register() to clean-up stuff in case of failure. Isn't
it a better interface?
--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
On Wed, Jun 03 2009, Artem Bityutskiy wrote:
> Jens Axboe wrote:
>>> Just to make sure I understood you correctly. I assume my original
>>> patch is fine (because there is bdi_destroy()) and merge it to
>>> ubifs tree.
>>
>> It needs to be:
>>
>> err = bdi_register(&c->bdi, NULL, "ubifs");
>> if (err)
>> goto out_bdi;
>>
>> so you hit the bdi_destroy() for that failure, not goto out_close;
>> Otherwise it was fine.
>
> Ah, I see. Rather non-typical convention though. I expected
> bdi_register() to clean-up stuff in case of failure. Isn't
> it a better interface?
You already did a bdi_init() at that point. bdi_destroy() must be used
to clean up after both bdi_init() and/or bdi_register().
--
Jens Axboe
Jens Axboe wrote:
>> Ah, I see. Rather non-typical convention though. I expected
>> bdi_register() to clean-up stuff in case of failure. Isn't
>> it a better interface?
>
> You already did a bdi_init() at that point. bdi_destroy() must be used
> to clean up after both bdi_init() and/or bdi_register().
Right, silly me.
--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
Jens Axboe wrote:
>> Just to make sure I understood you correctly. I assume my original
>> patch is fine (because there is bdi_destroy()) and merge it to
>> ubifs tree.
>
> It needs to be:
>
> err = bdi_register(&c->bdi, NULL, "ubifs");
> if (err)
> goto out_bdi;
>
> so you hit the bdi_destroy() for that failure, not goto out_close;
> Otherwise it was fine.
Did this, also added a
Reviewed-by: Jens Axboe <[email protected]>
http://git.infradead.org/ubifs-2.6.git?a=commit;h=813fdc16ad591e79d0c1b88d31970dcd1c2aa3f1
Thanks.
--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
On Wed, Jun 03 2009, Artem Bityutskiy wrote:
> Jens Axboe wrote:
>>> Just to make sure I understood you correctly. I assume my original
>>> patch is fine (because there is bdi_destroy()) and merge it to
>>> ubifs tree.
>>
>> It needs to be:
>>
>> err = bdi_register(&c->bdi, NULL, "ubifs");
>> if (err)
>> goto out_bdi;
>>
>> so you hit the bdi_destroy() for that failure, not goto out_close;
>> Otherwise it was fine.
>
> Did this, also added a
> Reviewed-by: Jens Axboe <[email protected]>
>
> http://git.infradead.org/ubifs-2.6.git?a=commit;h=813fdc16ad591e79d0c1b88d31970dcd1c2aa3f1
Looks good!
--
Jens Axboe