> > + if (XFS_TEST_ERROR(false, pag->pag_mount, XFS_ERRTAG_IUNLINK_FALLBACK)) > > + return 0; > > + > > + iu = kmem_zalloc(sizeof(*iu), KM_SLEEP | KM_NOFS); > > + iu->iu_agino = prev_agino; > > + iu->iu_next_unlinked = this_agino; > > + > > + error = rhashtable_insert_fast(&pag->pagi_unlinked_hash, > > + &iu->iu_rhash_head, xfs_iunlink_hash_params); > > + if (error == 0 || error == -EEXIST) > > + return error; > > + return 0; > > The return val looks funny at a glance: return -EEXIST or otherwise > return 0 for success and all other errors (also the error == 0 looks > superfluous). I see the comment above describes what this does, but it > doesn't explain why. I'd move the comment to above the if check and > explain why it's there (i.e., "Absorb all runtime errors besides -EEXIST > because fallback blah blah. We care about -EEXIST because ..."). > > Also I assume we need to free the iu object on insert failure, > regardless of whether we ultimately return the error. But is this really the right thing to do? Everything but ENOMEM would be a bug in the rhashtable implementation, or the XFS use, wouldn't it? So why not drop the return value entirely and do a: error = rhashtable_insert_fast(&pag->pagi_unlinked_hash, &iu->iu_rhash_head, xfs_iunlink_hash_params); ASSERT(error == 0 || error == -ENOMEM); > > + error = rhashtable_remove_fast(&pag->pagi_unlinked_hash, > > + &iu->iu_rhash_head, xfs_iunlink_hash_params); > > + if (error) > > + return error; > > What's interesting about remove failure is that if we otherwise found an > iu for this inode, removing the inode from the unlinked list leaves a > stale/incorrect iu around in the in-core table. So it's one thing for > the in-core table to be a valid subset of the on-disk structures, but > another thing entirely to have incoherence between the two. It might be > worth pointing out that it's critical to return an error here so we > don't actually remove the inode (whereas the re-add below is less > strict). Again, remove failure here sounds like a programming bug - so ASSERT and or forced shutdown here.
