xfs
[Top] [All Lists]

Re: [PATCH v2 2/2] xfs: fold xfs_create_tmpfile() into xfs_create()

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH v2 2/2] xfs: fold xfs_create_tmpfile() into xfs_create()
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Thu, 10 Apr 2014 08:19:56 -0400
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20140410102901.GB17641@xxxxxxxxxxxxx>
References: <1397071311-28371-1-git-send-email-bfoster@xxxxxxxxxx> <1397071311-28371-3-git-send-email-bfoster@xxxxxxxxxx> <20140410102901.GB17641@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Thu, Apr 10, 2014 at 03:29:01AM -0700, Christoph Hellwig wrote:
> > -   struct xfs_trans_res    tres;
> > +   struct xfs_trans_res    *tres;
> >     uint                    resblks;
> >  
> >     trace_xfs_create(dp, name);
> > @@ -1181,14 +1181,21 @@ xfs_create(
> >     if (is_dir) {
> >             rdev = 0;
> >             resblks = XFS_MKDIR_SPACE_RES(mp, name->len);
> > -           tres.tr_logres = M_RES(mp)->tr_mkdir.tr_logres;
> > -           tres.tr_logcount = XFS_MKDIR_LOG_COUNT;
> > +           tres = &M_RES(mp)->tr_mkdir;
> 
> The (nice) reservation cleanup should be a patch of it's own. 
> 

Ok.

> > +   } else {
> > +           /*
> > +            * If we don't have a name, we're in the ->tmpfile() path. We
> > +            * have a unique transaction here since we modify the unlinked
> > +            * list rather than create a directory entry.
> > +            */
> 
> How is that transaction more "uniqueue" than the others?  Seems like
> this comment generally doesn't add a whole lot of value.
> 

It's just as unique as the others. ;) I wasn't intending to call out
this transaction as special in any way. Rather, I was just trying to
document why there is a separate transaction depending on the existence
of the name. I can drop the comment.

> > +   if (name) {
> > +           xfs_ilock(dp, XFS_ILOCK_EXCL | XFS_ILOCK_PARENT);
> > +           unlock_dp_on_error = true;
> > +
> > +           error = xfs_dir_canenter(tp, dp, name, resblks);
> > +           if (error)
> > +                   goto out_trans_cancel;
> > +   }
> 
> So we get another special case in this function.  Can't say I like that
> too much, on the other hand I don't really like the duplicate code
> either.  So I'm not excited about this, but also not strongly against it.
> 

Indeed. I debated whether it would be reasonable to make this function
slightly longer and more complex on its own. When I realized
xfs_create_tmpfile() was 90% duplicate, it seemed worth the tradeoff for
a 100+ line function.

Brian

<Prev in Thread] Current Thread [Next in Thread>