xfs
[Top] [All Lists]

Re: [PATCH 03/14] xfs: convert log grant heads to LSN notation

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 03/14] xfs: convert log grant heads to LSN notation
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Thu, 2 Dec 2010 06:47:26 -0500
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <20101202020132.GP16922@dastard>
References: <1290994712-21376-1-git-send-email-david@xxxxxxxxxxxxx> <1290994712-21376-4-git-send-email-david@xxxxxxxxxxxxx> <20101201130504.GA18379@xxxxxxxxxxxxx> <20101202020132.GP16922@dastard>
User-agent: Mutt/1.5.21 (2010-09-15)
On Thu, Dec 02, 2010 at 01:01:32PM +1100, Dave Chinner wrote:
> On Wed, Dec 01, 2010 at 08:05:04AM -0500, Christoph Hellwig wrote:
> > > -STATIC int        xlog_space_left(xlog_t *log, int cycle, int bytes);
> > > +STATIC int        xlog_space_left(int logsize, xfs_lsn_t tail_lsn,
> > > +                         xfs_lsn_t head);
> > 
> > Looking further through the series I have to say I really hate
> > passing in the logsize instead of the log structure.  Passing the
> > log pointer from higher level functions just makes a lot more sense.
> > 
> > Also in this case passing the tail_lsn explicitly doesn't make any sense
> > - it becomes atomic later and thus there is no locking requirement for
> > it.
> 
> What I wanted to make clear is that the calculation works on fixed
> values and doesn't sample values internally itself. I guess that's
> not important for the log size, but for stuff like the tail lsn
> it avoids  needing to sample inside xlog_space_left() before we
> crack it. i.e. something like this is wrong:
> 
>       cycle = CYCLE_LSN(atomic64_read(&log->l_tail_lsn));
>       block = BLOCK_LSN(atomic64_read(&log->l_tail_lsn));
> 
> and this is correct:
> 
>       tail_lsn = atomic64_read(&log->l_tail_lsn);
>       cycle = CYCLE_LSN(tail_lsn);
>       block = BLOCK_LSN(tail_lsn);
> 
> So it makes sense to me to have the value of of the tail lsn and
> other variables that should only be sampled once passed into the
> function. That avoids misunderstandings further down the track
> because it is obvious that the calculation works on constant values.
> Perhaps I should add "const" to the parameter declarations to help
> make my intentions clear...

I don't think obsfucating the code is a good idea to reach this goal.

What might be better is a helper like:

static inline void xlog_crack_lsn(atomic64_t *lsn, int *cycle, int *block)
{
        xfs_lsn_t = atomic64_read(lsn);

        *cycle = CYCLE_LSN(tail_lsn);
        *block = BLOCK_LSN(tail_lsn);
}

and a long comment explaining how it needs to be used.

> I thought about that - my first version even did this. I thought it
> was easier to understand the changes if I didn't change the calling
> conventions for modifying the grant heads. As such, I'd prefer to
> make this change to the wrappers in a separate patch.

Heh, when looking at the patch I actually found this part pretty hard
to read already.  So moving the factoring of the helpers out into a
separate patch might indeed be a good idea, and that patch can also
remove the wrappers.

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