jbd2 : Fix journal start by passing a parameter to specify if the caller can deal with ENOMEM

Pass extra bool parameter in journal routines to specify if its ok to fail in the journal start. Passing true means caller is
ok with journal start failures and can handle ENOMEM. Update ocfs2 and ext4 routines to pass false for the updated journal interface by default to retain the existing behavior.

Signed-off-by Manish Katiyar <[email protected]>

Acked-by Jan Kara <[email protected]>

fs/jbd2/transaction.c +1 -1

@@ -318,7 +320,7 @@ static handle_t *new_handle(int nblocks)
  *
  * Return a pointer to a newly allocated handle, or NULL on failure
  */
-handle_t *jbd2__journal_start(journal_t *journal, int nblocks, int gfp_mask)
+handle_t *jbd2_journal_start(journal_t *journal, int nblocks, bool errok)
 {
 	handle_t *handle = journal_current_handle();
 	int err;

This patch is OK as such but as I wrote in a reply to a later patch in the series, you also need to pass 'errok' to start_this_handle() instead of gfp_mask as well. Then in start_this_handle() you can use 'errok' to decide whether we should retry the allocation or not instead of gfp_mask. And just call kzalloc in start_this_handle() with GFP_NOFS... Thanks.

Quoted 5 lines · Jan Kara

This patch is OK as such but as I wrote in a reply to a later patch in the series, you also need to pass 'errok' to start_this_handle() instead of gfp_mask as well. Then in start_this_handle() you can use 'errok' to decide whether we should retry the allocation or not instead of gfp_mask. And just call kzalloc in start_this_handle() with GFP_NOFS... Thanks.

Hi Jan,

ok.. I will do it as a separate patch.

Quoted 2 lines

ok.. I will do it as a separate patch.

Well, patch 2/3 does not really make too much sense without it (errok
parameter isn't used) so there's no reason to do it as a separate patch. Just add it to this patch please.

Quoted 5 lines · Jan Kara

Quoted 2 lines

ok.. I will do it as a separate patch.

Well, patch 2/3 does not really make too much sense without it (errok parameter isn't used) so there's no reason to do it as a separate patch. Just add it to this patch please.

Agreed; right now this whole patch series is a no-op, since errok isn't getting used for anything. So fixing errok so it's passed to start_this_handle() seems to be more in the category of "fix the patch" more than anything else.

One more thing; perhaps we should be passing in a integer so we can pass in a flag word. That way you don't need to have a fail_ok variant. It's a lot more obvious if you have a call:

	  handle = ext4_journal_start(inode, 1, JBD2_FAIL_OK);

What we can also do is this:

	  handle = ext4_journal_start(inode, 1, JBD2_FAIL_OK | JBD2_TOPLEVEL);

What JBD2_TOPLEVEL means is that caller is from a top-level file system function, such as ext4_symlink() or ext4_chmod(), such that start_this_handle() can use GFP_KERNEL instead of GFP_NOFS. GFP_NOFS is needed for any function that might get called by the direct reclaim path (i.e., the writepage() function). But for the top-level symlink() or chmod() function, it's actually OK to allocate memory using GFP_KERNEL, since there's no potential recursion problem.

Quoted 7 lines

What JBD2_TOPLEVEL means is that caller is from a top-level file system function, such as ext4_symlink() or ext4_chmod(), such that start_this_handle() can use GFP_KERNEL instead of GFP_NOFS. GFP_NOFS is needed for any function that might get called by the direct reclaim path (i.e., the writepage() function). But for the top-level symlink() or chmod() function, it's actually OK to allocate memory using GFP_KERNEL, since there's no potential recursion problem.

At this point, why not just pass GFP_KERNEL or GFP_NOFS directly, optionally with __GFP_NOFAIL?

Quoted 10 lines · Andreas Dilger

Quoted 7 lines

What JBD2_TOPLEVEL means is that caller is from a top-level file system function, such as ext4_symlink() or ext4_chmod(), such that start_this_handle() can use GFP_KERNEL instead of GFP_NOFS. GFP_NOFS is needed for any function that might get called by the direct reclaim path (i.e., the writepage() function). But for the top-level symlink() or chmod() function, it's actually OK to allocate memory using GFP_KERNEL, since there's no potential recursion problem.

At this point, why not just pass GFP_KERNEL or GFP_NOFS directly, optionally with __GFP_NOFAIL?

Well, __GFP_NOFAIL is going away. (At least there are a number of mm hackers, including akpm, who really want it to go away).

We could key off of GFP_NOFS, but GFP_NOFS doesn't mean loop and make sure that we don't fail. The two concepts are in fact orthogonal; it's just at the moment that there are most places which are called in the fs writeback path which also can't fail.

But just to give one example, ext4_bio_write_page() is an example of a function that allocates memory GFP_NOFS, but can fail with ENOMEM, because its caller, mpage_da_submit_io() in fs/ext4/inode.c is designed to cope with failure in a way that doesn't cause data loss. (We leave the page dirty, unlock it and back out of the writeback code, and later the bdi flusher threads will retry the writepages request.)

- Ted

P.S. That means that there are calls to ext4_journal_start() in the ext4_da_writepages() code path that probably could be converted to ext4_journal_start_failok() --- or to ext4_journal_start(inode, nblocks, JBD2_FAIL_OK) per my suggestion --- once we have fully audited the error return paths.

P.P.S. Something that might be worth doing is having a sysfs tunable that causes ext4_journal_start() to return an ENOMEM failure on a per file system basis with a probability specified by the sysfs tunable. This would allow us to actually _test_ the error handling to make sure sane things happen....

What I'd probably do is define a new flag, JBD2_WRITEBACK_TEST, and use it to make the ext4_journal_start() functions that are allowed to probabilistically fail (since the retry should be happening at a higher level), and then run a stress test with the syfs tunable enabled. Since the flag would only cause ext4_journal_start() functions that should have automatic fallbacks, the stress test would be able to run to completion even though 10% of the ext4_journal_start(... JBD2_WRITEBACK_TEST) calls were failing. That's another example of why using a flag bitfield instead of a bool is much more powerful.

No need to do this. If you make JBD2 use a separate slab for transaction
structures (trivial and makes some sense anyway), you can use fault-injection framework to do exactly what you describe above (see Documentation/fault-injection/fault-injection.txt and look for failslab).

But if we just fail all transaction allocations with say 10% probability,
it should work as well, shouldn't it? We'd just retry those allocations whose failure we cannot handle and eventually succeed. Or do I miss something?

Quoted 4 lines · Jan Kara

But if we just fail all transaction allocations with say 10% probability,
it should work as well, shouldn't it? We'd just retry those allocations whose failure we cannot handle and eventually succeed. Or do I miss something?

The reason why I only wanted to fail the transactions relating to the writeback path is because other failures will get reflected back to userspace, and would thus change the behavior of the stress test. (If we used fsstress, it would cause fsstress to immediately stop and fail, for example.)

That is the one thing that worries me a little about this patch series in general. If we suddenly start failing open() or rename() or chmod() syscalls with ENOMEM in low memory situations, what of programs that aren't doing adequate error checking? Sure, other file systems will do this, but the bulk of the users use ext3/ext4, and remember how much kvetching and complaining when xfs was the first file system to require user space applications to actually use fsync() if they wanted their files to be safe after a power failure.

I worry that there are a lot of incompetently written editors out there that aren't doing error checking, or worse yet, package managers or other security-critical programs that aren't doing error checking, and which won't notice when an syscall fails in a low-memory situation, leading to either (a) user data loss (which the application programers will lay at the feet of the file system developers, don't doubt it), or (b) security holes.

Quoted 4 lines · Jan Kara

  No need to do this. If you make JBD2 use a separate slab for transaction structures (trivial and makes some sense anyway), you can use fault-injection framework to do exactly what you describe above (see Documentation/fault-injection/fault-injection.txt and look for failslab).

Thanks for pointing me at the fault-injection framework; it's not something I've used before.  I'll have to take a look at it.

Quoted 4 lines

  But if we just fail all transaction allocations with say 10% probability, it should work as well, shouldn't it? We'd just retry those allocations whose failure we cannot handle and eventually succeed. Or do I miss something?

The reason why I only wanted to fail the transactions relating to the writeback path is because other failures will get reflected back to userspace, and would thus change the behavior of the stress test.  (If we used fsstress, it would cause fsstress to immediately stop and fail, for example.)

That is the one thing that worries me a little about this patch series in general.  If we suddenly start failing open() or rename() or chmod() syscalls with ENOMEM in low memory situations, what of programs that aren't doing adequate error checking?  Sure, other file systems will do this, but the bulk of the users use ext3/ext4, and remember how much kvetching and complaining when xfs was the first file system to require user space applications to actually use fsync() if they wanted their files to be safe after a power failure.

I worry that there are a lot of incompetently written editors out there that aren't doing error checking, or worse yet, package managers or other security-critical programs that aren't doing error checking, and which won't notice when an syscall fails in a low-memory situation, leading to either (a) user data loss (which the application programers will lay at the feet of the file system developers, don't doubt it), or (b) security holes.

I'm not sure there's a way to address this concern, and I'm going not NACK'ing this patch series on that basis --- but I do worry that it might not improve the situation by a whole lot, and may in fact cause some problems, at the end of the day.

Quoted 5 lines · Jan Kara

Quoted 2 lines

   ok.. I will do it as a separate patch.

  Well, patch 2/3 does not really make too much sense without it (errok parameter isn't used) so there's no reason to do it as a separate patch. Just add it to this patch please.

Agreed; right now this whole patch series is a no-op, since errok isn't getting used for anything.  So fixing errok so it's passed to start_this_handle() seems to be more in the category of "fix the patch" more than anything else.

One more thing; perhaps we should be passing in a integer so we can pass in a flag word.  That way you don't need to have a fail_ok variant.  It's a lot more obvious if you have a call:

         handle = ext4_journal_start(inode, 1, JBD2_FAIL_OK);

What we can also do is this:

         handle = ext4_journal_start(inode, 1, JBD2_FAIL_OK | JBD2_TOPLEVEL);

What JBD2_TOPLEVEL means is that caller is from a top-level file system function, such as ext4_symlink() or ext4_chmod(), such that start_this_handle() can use GFP_KERNEL instead of GFP_NOFS.  GFP_NOFS is needed for any function that might get called by the direct reclaim path (i.e., the writepage() function).  But for the top-level symlink() or chmod() function, it's actually OK to allocate memory using GFP_KERNEL, since there's no potential recursion problem.

                                                      - Ted

Quoted 22 lines · Ted Ts'o

Quoted 5 lines · Jan Kara

Quoted 2 lines

   ok.. I will do it as a separate patch.

  Well, patch 2/3 does not really make too much sense without it (errok parameter isn't used) so there's no reason to do it as a separate patch. Just add it to this patch please.

Agreed; right now this whole patch series is a no-op, since errok isn't getting used for anything.  So fixing errok so it's passed to start_this_handle() seems to be more in the category of "fix the patch" more than anything else.

One more thing; perhaps we should be passing in a integer so we can pass in a flag word.  That way you don't need to have a fail_ok variant.  It's a lot more obvious if you have a call:

         handle = ext4_journal_start(inode, 1, JBD2_FAIL_OK);

What we can also do is this:

         handle = ext4_journal_start(inode, 1, JBD2_FAIL_OK | JBD2_TOPLEVEL);

Quoted 7 lines

What JBD2_TOPLEVEL means is that caller is from a top-level file system function, such as ext4_symlink() or ext4_chmod(), such that start_this_handle() can use GFP_KERNEL instead of GFP_NOFS.  GFP_NOFS is needed for any function that might get called by the direct reclaim path (i.e., the writepage() function).  But for the top-level symlink() or chmod() function, it's actually OK to allocate memory using GFP_KERNEL, since there's no potential recursion problem.

Will it be more desirable to have the patch fixed as JBD2_FAIL_OK as you suggested above ? I have already
fixed the patch to pass errok to start_this_handle() and then retry or not based on that, and was planning to send
it. But if this needs to be fixed this way, then I will rework it then send.

Quoted 5 lines · Jan Kara

Quoted 2 lines

   ok.. I will do it as a separate patch.

  Well, patch 2/3 does not really make too much sense without it (errok parameter isn't used) so there's no reason to do it as a separate patch. Just add it to this patch please.

Agreed; right now this whole patch series is a no-op, since errok isn't getting used for anything.  So fixing errok so it's passed to start_this_handle() seems to be more in the category of "fix the patch" more than anything else.

One more thing; perhaps we should be passing in a integer so we can pass in a flag word.  That way you don't need to have a fail_ok variant.  It's a lot more obvious if you have a call:

         handle = ext4_journal_start(inode, 1, JBD2_FAIL_OK);

What we can also do is this:

         handle = ext4_journal_start(inode, 1, JBD2_FAIL_OK | JBD2_TOPLEVEL);

What JBD2_TOPLEVEL means is that caller is from a top-level file system function, such as ext4_symlink() or ext4_chmod(), such that start_this_handle() can use GFP_KERNEL instead of GFP_NOFS.  GFP_NOFS is needed for any function that might get called by the direct reclaim path (i.e., the writepage() function).  But for the top-level symlink() or chmod() function, it's actually OK to allocate memory using GFP_KERNEL, since there's no potential recursion problem.