Hi,
While going through jbd code, I was wondering why do we need to check
new_transaction for NULL, if we are passing __GFP_NOFAIL ?
Last code change around this code was when Ted converted kmalloc to
kzalloc, but since he also didn't remove it I am guessing there would
be some good reason for it. Can someone enlighten me ?
start_this_handle() {
..........
..........
new_transaction = kzalloc(sizeof(*new_transaction),
GFP_NOFS|__GFP_NOFAIL);
if (!new_transaction) {
ret = -ENOMEM;
goto out;
}
..........
}
Thanks -
Manish
On Sun, Mar 1, 2009 at 11:35 AM, Manish Katiyar <[email protected]> wrote:
> Hi,
>
> While going through jbd code, I was wondering why do we need to check
> new_transaction for NULL, if we are passing __GFP_NOFAIL ?
> Last code change around this code was when Ted converted kmalloc to
> kzalloc, but since he also didn't remove it I am guessing there would
> be some good reason for it. Can someone enlighten me ?
I didn't receive any response to this. So probably removing the NULL
check is harmless. Or should I remove the __GFP_NOFAIL flag and keep
the error handling ?
Patches to follow.
Thanks -
Manish
>
> start_this_handle() {
> ..........
> ? ? ? ?..........
> ? ? ? ?new_transaction = kzalloc(sizeof(*new_transaction),
> ? ? ? ? ? ? ? ? ? ? ? ?GFP_NOFS|__GFP_NOFAIL);
> ? ? ? ?if (!new_transaction) {
> ? ? ? ? ? ? ? ?ret = -ENOMEM;
> ? ? ? ? ? ? ? ?goto out;
> ? ? ? ?}
> ? ? ? ?..........
> }
>
>
> Thanks -
> Manish
>
On Mar 02, 2009 11:38 +0530, Manish Katiyar wrote:
> On Sun, Mar 1, 2009 at 11:35 AM, Manish Katiyar <[email protected]> wrote:
> > While going through jbd code, I was wondering why do we need to check
> > new_transaction for NULL, if we are passing __GFP_NOFAIL ?
> > Last code change around this code was when Ted converted kmalloc to
> > kzalloc, but since he also didn't remove it I am guessing there would
> > be some good reason for it. Can someone enlighten me ?
>
> I didn't receive any response to this. So probably removing the NULL
> check is harmless. Or should I remove the __GFP_NOFAIL flag and keep
> the error handling ?
Neither, please. The NULL check is harmless, and static code checkers
will complain about k[zm]alloc() without a corresponding NULL check.
Branch prediction will get this right, so the overhead is miniscule.
> > start_this_handle() {
> > ..........
> > ? ? ? ?..........
> > ? ? ? ?new_transaction = kzalloc(sizeof(*new_transaction),
> > ? ? ? ? ? ? ? ? ? ? ? ?GFP_NOFS|__GFP_NOFAIL);
> > ? ? ? ?if (!new_transaction) {
> > ? ? ? ? ? ? ? ?ret = -ENOMEM;
> > ? ? ? ? ? ? ? ?goto out;
> > ? ? ? ?}
> > ? ? ? ?..........
> > }
Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.
On Mon, Mar 2, 2009 at 1:05 PM, Andreas Dilger <[email protected]> wrote:
> On Mar 02, 2009 ?11:38 +0530, Manish Katiyar wrote:
>> On Sun, Mar 1, 2009 at 11:35 AM, Manish Katiyar <[email protected]> wrote:
>> > While going through jbd code, I was wondering why do we need to check
>> > new_transaction for NULL, if we are passing __GFP_NOFAIL ?
>> > Last code change around this code was when Ted converted kmalloc to
>> > kzalloc, but since he also didn't remove it I am guessing there would
>> > be some good reason for it. Can someone enlighten me ?
>>
>> I didn't receive any response to this. So probably removing the NULL
>> check is harmless. Or should I remove the __GFP_NOFAIL flag and keep
>> the error handling ?
>
> Neither, please. ?The NULL check is harmless, and static code checkers
> will complain about k[zm]alloc() without a corresponding NULL check.
Ohh... Ok.. Thanks Andreas, I didn't think about static code checkers.
Please ignore the sent patch.
Thanks -
Manish
> Branch prediction will get this right, so the overhead is miniscule.
>
>> > start_this_handle() {
>> > ..........
>> > ? ? ? ?..........
>> > ? ? ? ?new_transaction = kzalloc(sizeof(*new_transaction),
>> > ? ? ? ? ? ? ? ? ? ? ? ?GFP_NOFS|__GFP_NOFAIL);
>> > ? ? ? ?if (!new_transaction) {
>> > ? ? ? ? ? ? ? ?ret = -ENOMEM;
>> > ? ? ? ? ? ? ? ?goto out;
>> > ? ? ? ?}
>> > ? ? ? ?..........
>> > }
>
> Cheers, Andreas
> --
> Andreas Dilger
> Sr. Staff Engineer, Lustre Group
> Sun Microsystems of Canada, Inc.
>
>
On Mon, Mar 02, 2009 at 11:38:53AM +0530, Manish Katiyar wrote:
> On Sun, Mar 1, 2009 at 11:35 AM, Manish Katiyar <[email protected]> wrote:
> > Hi,
> >
> > While going through jbd code, I was wondering why do we need to check
> > new_transaction for NULL, if we are passing __GFP_NOFAIL ?
> > Last code change around this code was when Ted converted kmalloc to
> > kzalloc, but since he also didn't remove it I am guessing there would
> > be some good reason for it. Can someone enlighten me ?
>
> I didn't receive any response to this. So probably removing the NULL
> check is harmless. Or should I remove the __GFP_NOFAIL flag and keep
> the error handling ?
We don't want to remove __GFP_NOFAIL here since returning an error
would cause the filesystem to be marked as corrupted, and so it's
better to simply ask the memory allocator to retry until it can
succeed, which is what GFP_NOFAIL does.
This isn't an excuse for using GFP_NOFAIL in journal_init_common(),
though; that's an example of a use of __GFP_NOFAIL that should go
away. (Basically, if that fails, the mount will fail; we should
printk an explanatory message since the mount system call return
EINVAL, but failing a mount due to not enough memory is reasonable.
Marking the filesystem as corrupted, and remounting the filesystem
read-only and/or panic'ing the system is not.)
- Ted