Hi Bob, On Fri, Jan 3, 2020 at 4:33 PM Bob Peterson <[email protected]> wrote: > Before this patch set, several gfs2 functions failed to reserve enough > revoke entries in the journal. Some examples: > > 1. gfs2_dinode_dealloc failed to reserve a revoke for the dinode > being deleted.
that sounds like a bug. > 2. Any function that allocates dinodes with gfs2_alloc_blocks > should reserve a revoke because alloc_blocks will premptively > call trans_remove_revoke to make sure there isn't a pending revoke > for the new dinode. > 3. Any function that potentially will unstuff a stuffed directory > needs to reserve a revoke because gfs2_unstuff_dinode calls > gfs2_trans_remove_revoke for the new journaled leaf block. Removing revokes doesn't require any space in the journal, so why should we need to reserve space in that case? > In addition, function gfs2_trans_remove_revoke unconditionally > decrements tr->tr_num_revoke, and if not enough revokes are reserved, the > value goes from 0 to 4294967295 (-1, but it's an unsigned int). This is later > re-added to the system-wide revoke numbers, thereby decrementing the value > (sd_log_commited_revoke) "properly," but by accident. This worked properly > most of the time because one transaction would reserve space for revokes, > then it would be merged with the system transaction (sdp->sd_log_tr) and it > usually did not run out, because you can hold a lot of revoke entries > per log descriptor block. Some of the code, such as gfs2_write_revokes, would > work around this and somehow got it right most of the time. However, some > jdata tests with xfstests generic/269 encountered problems when it actually > ran out. Would something like this work to fix the revoke accounting? --- a/fs/gfs2/trans.c +++ b/fs/gfs2/trans.c @@ -256 +255,0 @@ void gfs2_trans_remove_revoke(struct gfs2_sbd *sdp, u64 blkno, unsigned int len) - struct gfs2_trans *tr = current->journal_info; @@ -268 +267 @@ void gfs2_trans_remove_revoke(struct gfs2_sbd *sdp, u64 blkno, unsigned int len) - tr->tr_num_revoke--; + bd->bd_tr->tr_num_revoke--; Thanks, Andreas
