Re: [PATCH 2/3] libxlog: sync up with 2.6.38 kernel code

Alex Elder <aelder@xxxxxxx> · Wed, 09 Feb 2011 15:49:03 -0600

On Mon, 2011-01-10 at 19:44 +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> Update libxlog with the current 2.6.38 kernel code and well as
> updating the necessary parts of libxfs and variaous header files to
> ensure that it compiles correctly.

OK, I went through this fairly exhaustively.  That being
said, I only verified that the stuff you are changing
makes sense as compared to the stuff that was there
before.  I have not done any significant checks to
try to be sure the existing code was all good.

At a very high level, I think this all looks good.

It points out that there is (or will be) a fairly
small set of things that need to really differ
between the kernel and libxfs/libxfslog code.
I don't expect we'll ever truly converge on one
set of code, but we probably could come closer to
that with a little restructuring of some things.

Since so much of this is code shared with the
kernel, I think it should be pretty solid.  I
have a little bit of concern about the things
that do differ--specifically the problems you
note in the next patch about the log sector
size and the inode reference counting.  It
seems that ought to get a bit more scrutiny
or testing somehow.  Stuff in "libxfs/logitem.c"
and "libxfs_trans.c" I didn't really think too
hard about, for example.

Otherwise, I guess I'll say that I've reviewed
this code and, with perhaps a little bit of
trepidation say it looks OK.

Reviewed-by: Alex Elder <aelder@xxxxxxx>

> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
>  include/Makefile           |    5 +-
>  include/atomic.h           |   31 ++
>  include/hlist.h            |   76 +++++
>  include/libxfs.h           |   17 +-
>  include/libxlog.h          |   16 +-
>  include/list.h             |   11 +
>  include/xfs_buf_item.h     |   50 ++---
>  include/xfs_extfree_item.h |   17 +-
>  include/xfs_inode_item.h   |    2 -
>  include/xfs_log.h          |   70 +++--
>  include/xfs_log_priv.h     |  331 +++++++++++++++++------
>  include/xfs_log_recover.h  |   25 +-
>  include/xfs_trace.h        |    9 +
>  include/xfs_trans.h        |  648 ++++++--------------------------------------
>  include/xfs_types.h        |    2 +
>  libxfs/logitem.c           |  365 +------------------------
>  libxfs/trans.c             |  209 +++++----------
>  libxfs/xfs.h               |   12 +-
>  libxfs/xfs_mount.c         |    1 -
>  libxfs/xfs_trans.c         |  492 ++++++++++++++++++++++++++++++---
>  libxlog/xfs_log_recover.c  |  606 ++++++++++++++++++++++-------------------
>  logprint/log_print_all.c   |   13 +-
>  logprint/log_print_trans.c |    4 +-
>  23 files changed, 1431 insertions(+), 1581 deletions(-)
>  create mode 100644 include/atomic.h
>  create mode 100644 include/hlist.h
>  create mode 100644 include/xfs_trace.h
> 

. . .

> diff --git a/include/hlist.h b/include/hlist.h
> new file mode 100644
> index 0000000..6d814ab
> --- /dev/null
> +++ b/include/hlist.h
> @@ -0,0 +1,76 @@
> +/*

This is a technicality unrelated to the code itself.

No copyright ownership assertion?  I realize it was
taken from the kernel headers and therefore you don't
want to pretend it's your work.  But on the other hand,
shouldn't *someone* own this particular set of bits?

> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it would be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write the Free Software Foundation,
> + * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + */
> +#ifndef __HLIST_H__
> +#define __HLIST_H__
> +
> +/*
> + * double-linked hash list with single head implementation taken from linux
> + * kernel headers.

Maybe you could add a commit id, or perhaps just a kernel
version (like 2.6.37) to make more precise the version from
which this code derives (as long as you're creating a new
file to contain this).

> + */
> +
> +struct hlist_node {
> +	struct hlist_node *next;
> +	struct hlist_node **pprev;
> +};

. . .

> diff --git a/include/libxfs.h b/include/libxfs.h
> index 13fbf79..69b24ac 100644
> --- a/include/libxfs.h
> +++ b/include/libxfs.h

. . .

> @@ -330,6 +340,7 @@ typedef struct xfs_inode_log_item {
>  	unsigned short		ili_flags;		/* misc flags */
>  	unsigned int		ili_last_fields;	/* fields when flushed*/
>  	xfs_inode_log_format_t	ili_format;		/* logged structure */
> +	int			ili_lock_flags;
>  } xfs_inode_log_item_t;

There is a file "include/xfs_inode_item.c" present in the
xfsprogs tree (and it would otherwise also need this field
added to it).  As far as I can tell, nobody uses it.  Do
you know why it's there?  Would you mind including its
deletion along with this change?  (There may be other
similar unused files, but I won't worry about that for
now.)

>  typedef struct xfs_buf_log_item {
> @@ -353,7 +364,7 @@ typedef struct xfs_trans {
>  	long		t_fdblocks_delta;	/* superblock fdblocks chg */
>  	long		t_frextents_delta;	/* superblock freextents chg */
>  	unsigned int	t_items_free;		/* log item descs free */
> -	xfs_log_item_chunk_t	t_items;	/* first log item desc chunk */
> +	struct list_head	t_items;	/* first log item desc chunk */
>  } xfs_trans_t;

Here too, file "include/xfs_trans.h" also defines xfs_trans_t
but doesn't have a t_items field.  The header file itself does
not appear to be used.  Maybe we do need to do a sweep through
all the files and kill off the ones that are never used.  Do
you know the history here, whether/why files like that were
ever used?  Maybe they are used, and I'm somehow missing it
with my cscope database.

. . .

> diff --git a/include/xfs_extfree_item.h b/include/xfs_extfree_item.h
> index 2f049f6..375f68e 100644
> --- a/include/xfs_extfree_item.h
> +++ b/include/xfs_extfree_item.h

I think it would be worth noting in the commit message that
this file is becoming identical to the file in the kernel
at "fs/xfs/xfs_extfree_item.h".  This is the case for a
number of other files as well (I'll try to call attention
to them as I go through).

Actually, I dug a little deeper on this and I see it's
a bit more complicated than that.  Many of the files
are becoming identical to their kernel counterparts.
I'll send something out later with some analysis on that.

> diff --git a/include/xfs_log.h b/include/xfs_log.h
> index d47b91f..916eb7d 100644
> --- a/include/xfs_log.h
> +++ b/include/xfs_log.h

The latest change to this file in the kernel source
changes the return type of xfs_log_commit_cil() to
void (not reflected in this version for user space).

. . .

> diff --git a/include/xfs_trace.h b/include/xfs_trace.h
> new file mode 100644
> index 0000000..fef82d0
> --- /dev/null
> +++ b/include/xfs_trace.h

You should add a boilerplate copyright and license
section to this file.  I realize this will probably
triple its size...

. . .

> diff --git a/libxfs/logitem.c b/libxfs/logitem.c
> index d6ef10b..0856ca6 100644
> --- a/libxfs/logitem.c
> +++ b/libxfs/logitem.c

Stuff in this file I find is generally quite different
from what's in the kernel.  I'm going to chalk that up
to some of the stuff that still needs to be sorted out
between kernel and user space reference counting model.

I have looked at the differences in the hunks here and
unless I've noted it I've tried to verify that the
change looks sensible.

. . .

> @@ -250,149 +41,21 @@ xfs_trans_buf_item_match(

This appears to be identical to the one in the
kernel, but the spacing is slightly different and
the header comment differs a bit.  You should
make them actually identical.

. . .

> diff --git a/libxfs/trans.c b/libxfs/trans.c
> index 1c60f38..c5dd2ca 100644
> --- a/libxfs/trans.c
> +++ b/libxfs/trans.c
> @@ -36,8 +36,7 @@ libxfs_trans_alloc(
>  	}
>  	ptr->t_mountp = mp;
>  	ptr->t_type = type;
> -	ptr->t_items_free = XFS_LIC_NUM_SLOTS;

t_items_free is removed from the xfs_trans structure
in "include/xfs_trans.h" but it remains in the version
of that structure defined in "include/libxfs.h".

I believe the latter is the one we want to keep,
but if that's the case you'd better delete the
field definition there.  (The compiler isn't
warning you that you neglected to initialize a
field.)

> -	xfs_lic_init(&ptr->t_items);
> +	INIT_LIST_HEAD(&ptr->t_items);
>  #ifdef XACT_DEBUG
>  	fprintf(stderr, "allocated new transaction %p\n", ptr);
>  #endif

. . .

> @@ -224,19 +235,14 @@ xfs_trans_log_inode(
>  	xfs_inode_t		*ip,
>  	uint			flags)
>  {
> -	xfs_log_item_desc_t	*lidp;
> -
>  	ASSERT(ip->i_transp == tp);
>  	ASSERT(ip->i_itemp != NULL);
>  #ifdef XACT_DEBUG
>  	fprintf(stderr, "dirtied inode %llu, transaction %p\n", ip->i_ino, tp);
>  #endif

The only difference between this function and the
one in the kernel is that the kernel has an assert
here (xfs_isilocked()) rather than the debug fprintf().

. . .

> diff --git a/libxfs/xfs.h b/libxfs/xfs.h
> index 8e94dad..a9e2bf1 100644
> --- a/libxfs/xfs.h
> +++ b/libxfs/xfs.h

This file is *nearly* identical to its kernel
counterpart.  Maybe those last few differences
can get resolved soon.

In any case, this file looks good.

. . .

> diff --git a/libxfs/xfs_mount.c b/libxfs/xfs_mount.c
> index 02bff42..dde9678 100644
> --- a/libxfs/xfs_mount.c
> +++ b/libxfs/xfs_mount.c
> @@ -270,7 +270,6 @@ xfs_mount_common(xfs_mount_t *mp, xfs_sb_t *sbp)
>  	mp->m_blockmask = sbp->sb_blocksize - 1;
>  	mp->m_blockwsize = sbp->sb_blocksize >> XFS_WORDLOG;
>  	mp->m_blockwmask = mp->m_blockwsize - 1;
> -	INIT_LIST_HEAD(&mp->m_del_inodes);

This hunk belongs in the next patch, not this one.

>  
>  	/*
>  	 * Setup for attributes, in case they get created.
> diff --git a/libxfs/xfs_trans.c b/libxfs/xfs_trans.c
> index 9036995..635de8f 100644
> --- a/libxfs/xfs_trans.c
> +++ b/libxfs/xfs_trans.c

This file looks good.

. . .

> diff --git a/libxlog/xfs_log_recover.c b/libxlog/xfs_log_recover.c
> index 9e0e567..23fe6fd 100644
> --- a/libxlog/xfs_log_recover.c
> +++ b/libxlog/xfs_log_recover.c

This file looks generally good, but I've made a few notes
below.

. . .

> @@ -82,26 +136,24 @@ xlog_bread(

. . .

> +int
> +xlog_bread(