[20/22] jbd2: Make credit checking more strict
diff mbox series

Message ID 20191003220613.10791-20-jack@suse.cz
State Superseded
Headers show
Series
  • ext4: Fix transaction overflow due to revoke descriptors
Related show

Commit Message

Jan Kara Oct. 3, 2019, 10:06 p.m. UTC
Make checking of available credits in jbd2_journal_dirty_metadata() more
strict. There should be always enough credits in the handle to write all
potential revoke descriptors. Also we warn in case there are not enough
credits since this is a bug in the filesystem.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/jbd2/transaction.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Theodore Y. Ts'o Oct. 21, 2019, 10:29 p.m. UTC | #1
On Fri, Oct 04, 2019 at 12:06:06AM +0200, Jan Kara wrote:
> Make checking of available credits in jbd2_journal_dirty_metadata() more
> strict. There should be always enough credits in the handle to write all
> potential revoke descriptors. Also we warn in case there are not enough
> credits since this is a bug in the filesystem.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

This is fine, but I wonder if we should also be returning an error in
jbd2_journal_revoke() --- of course, one problem is ext4_forget() is
getting called from ext4_free_blocks(), which currently doesn't return
an error.  But we can capture the error return in __ext4_forget(), and
at that point we can give a much more useful error message, since we
can print the function caller and line number.

Feel free to add:

Reviewed-by: Theodore Ts'o <tytso@mit.edu>
Jan Kara Oct. 23, 2019, 1:30 p.m. UTC | #2
On Mon 21-10-19 18:29:59, Theodore Y. Ts'o wrote:
> On Fri, Oct 04, 2019 at 12:06:06AM +0200, Jan Kara wrote:
> > Make checking of available credits in jbd2_journal_dirty_metadata() more
> > strict. There should be always enough credits in the handle to write all
> > potential revoke descriptors. Also we warn in case there are not enough
> > credits since this is a bug in the filesystem.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> 
> This is fine, but I wonder if we should also be returning an error in
> jbd2_journal_revoke() --- of course, one problem is ext4_forget() is
> getting called from ext4_free_blocks(), which currently doesn't return
> an error.  But we can capture the error return in __ext4_forget(), and
> at that point we can give a much more useful error message, since we
> can print the function caller and line number.

Yeah, that's a good point. I'll add a sanity check to jbd2_journal_revoke()
and then generate some error message in ext4.

> Feel free to add:
> 
> Reviewed-by: Theodore Ts'o <tytso@mit.edu>

Thanks!

								Honza

Patch
diff mbox series

diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
index 8851cbbe3579..66fad49d45df 100644
--- a/fs/jbd2/transaction.c
+++ b/fs/jbd2/transaction.c
@@ -1458,7 +1458,7 @@  int jbd2_journal_dirty_metadata(handle_t *handle, struct buffer_head *bh)
 		 * of the transaction. This needs to be done
 		 * once a transaction -bzzz
 		 */
-		if (handle->h_total_credits <= 0) {
+		if (WARN_ON_ONCE(jbd2_handle_buffer_credits(handle) <= 0)) {
 			ret = -ENOSPC;
 			goto out_unlock_bh;
 		}