On Wed, May 12, 2021 at 07:48:27PM +0200, David Sterba wrote:
> On Wed, May 05, 2021 at 12:20:43PM -0700, Boris Burkov wrote:
> > +/*
> > + * Helper to manage the transaction for adding an orphan item.
> > + */
> > +static int add_orphan(struct btrfs_inode *inode)
>
> I wonder if this helper is useful, it's used only once and the code is
> not long. Simply wrapping btrfs_orphan_add into a transaction is short
> enough to be in btrfs_begin_enable_verity.
>
I agree that just the plain transaction logic is not a big deal, and I
couldn't figure out how to phrase the comment so I left it at that,
which is unhelpful.
With that said, I found that pulling it out into a helper function
significantly reduced the gross-ness of the error handling in the
callsites. Especially for del_orphan in end verity which tries to
handle failures deleting the orphans, which quickly got tangled up with
other errors in the function and the possible transaction errors.
Honestly, I was surprised just how much it helped, and couldn't really
figure out why. If a helper being really beneficial is abnormal, I can
try again to figure out a clean way to write the code with the
transaction in-line.
> > +{
> > + struct btrfs_trans_handle *trans;
> > + struct btrfs_root *root = inode->root;
> > + int ret = 0;
> > +
> > + trans = btrfs_start_transaction(root, 1);
> > + if (IS_ERR(trans)) {
> > + ret = PTR_ERR(trans);
> > + goto out;
> > + }
> > + ret = btrfs_orphan_add(trans, inode);
> > + if (ret) {
> > + btrfs_abort_transaction(trans, ret);
> > + goto out;
> > + }
> > + btrfs_end_transaction(trans);
> > +
> > +out:
> > + return ret;
> > +}
> > +
> > +/*
> > + * Helper to manage the transaction for deleting an orphan item.
> > + */
> > +static int del_orphan(struct btrfs_inode *inode)
>
> Same here.
My comment is dumb again, but the nlink check does make this function
marginally more useful for re-use/correctness.
>
> > +{
> > + struct btrfs_trans_handle *trans;
> > + struct btrfs_root *root = inode->root;
> > + int ret;
> > +
> > + /*
> > + * If the inode has no links, it is either already unlinked, or was
> > + * created with O_TMPFILE. In either case, it should have an orphan from
> > + * that other operation. Rather than reference count the orphans, we
> > + * simply ignore them here, because we only invoke the verity path in
> > + * the orphan logic when i_nlink is 0.
> > + */
> > + if (!inode->vfs_inode.i_nlink)
> > + return 0;
> > +
> > + trans = btrfs_start_transaction(root, 1);
> > + if (IS_ERR(trans))
> > + return PTR_ERR(trans);
> > +
> > + ret = btrfs_del_orphan_item(trans, root, btrfs_ino(inode));
> > + if (ret) {
> > + btrfs_abort_transaction(trans, ret);
> > + return ret;
> > + }
> > +
> > + btrfs_end_transaction(trans);
> > + return ret;
> > +}
|