[U-Boot] [PATCH 2/8] JFFS2: Speed up and fix comparison functions

[Heiko Schocher denx]

Hello Mark,

Am 29.06.2015 um 07:02 schrieb Mark Tomlinson:
> Copying complete nodes from flash can be slow if the flash is slow
> to read. By only reading the data needed, the sorting operation can
> be made much faster.
>
> The directory entry comparison function also had a two bugs. First, it
> did not ensure the name was copied, so the name comparison may have
> been faulty (although it would have worked with NOR flash).  Second,
> setting the ino to zero to ignore the entry did not work, since this
> was either writing to a temporary buffer, or (for NOR flash) directly
> to flash. Either way, the change was not remembered.
>
> Signed-off-by: Mark Tomlinson <mark.tomlinson at alliedtelesis.co.nz>
> ---
>
>   fs/jffs2/jffs2_1pass.c | 82 ++++++++++++++++++++++++++------------------------
>   1 file changed, 42 insertions(+), 40 deletions(-)
>
> diff --git a/fs/jffs2/jffs2_1pass.c b/fs/jffs2/jffs2_1pass.c
> index 2335db1..079bb73 100644
> --- a/fs/jffs2/jffs2_1pass.c
> +++ b/fs/jffs2/jffs2_1pass.c
> @@ -598,14 +598,17 @@ insert_node(struct b_list *list, u32 offset)
>    */
>   static int compare_inodes(struct b_node *new, struct b_node *old)
>   {
> -	struct jffs2_raw_inode ojNew;
> -	struct jffs2_raw_inode ojOld;
> -	struct jffs2_raw_inode *jNew =
> -		(struct jffs2_raw_inode *)get_fl_mem(new->offset, sizeof(ojNew), &ojNew);
> -	struct jffs2_raw_inode *jOld =
> -		(struct jffs2_raw_inode *)get_fl_mem(old->offset, sizeof(ojOld), &ojOld);
> -
> -	return jNew->version > jOld->version;
> +	/* Only read in the version info from flash, not the entire inode.

please fix your comment style globally, thanks1

> +	 * This can make a big difference to speed if flash is slow.
> +	 */
> +	u32 new_version;
> +	u32 old_version;
> +	get_fl_mem(new->offset + offsetof(struct jffs2_raw_inode, version),
> +		   sizeof(new_version), &new_version);
> +	get_fl_mem(old->offset + offsetof(struct jffs2_raw_inode, version),
> +		   sizeof(old_version), &old_version);
> +
> +	return new_version > old_version;
>   }
>
>   /* Sort directory entries so all entries in the same directory
> @@ -615,42 +618,41 @@ static int compare_inodes(struct b_node *new, struct b_node *old)
>    */
>   static int compare_dirents(struct b_node *new, struct b_node *old)
>   {
> -	struct jffs2_raw_dirent ojNew;
> -	struct jffs2_raw_dirent ojOld;
> -	struct jffs2_raw_dirent *jNew =
> -		(struct jffs2_raw_dirent *)get_fl_mem(new->offset, sizeof(ojNew), &ojNew);
> -	struct jffs2_raw_dirent *jOld =
> -		(struct jffs2_raw_dirent *)get_fl_mem(old->offset, sizeof(ojOld), &ojOld);
> -	int cmp;
> -
> -	/* ascending sort by pino */
> -	if (jNew->pino != jOld->pino)
> -		return jNew->pino > jOld->pino;
> -
> -	/* pino is the same, so use ascending sort by nsize, so
> -	 * we don't do strncmp unless we really must.
> -	 */
> -	if (jNew->nsize != jOld->nsize)
> -		return jNew->nsize > jOld->nsize;
> -
> -	/* length is also the same, so use ascending sort by name
> -	 */
> -	cmp = strncmp((char *)jNew->name, (char *)jOld->name, jNew->nsize);
> -	if (cmp != 0)
> -		return cmp > 0;
> -
> -	/* we have duplicate names in this directory, so use ascending
> -	 * sort by version
> +	/* Using NULL as the buffer for NOR flash prevents the entire node
> +	 * being read. This makes most comparisons much quicker as only one
> +	 * or two entries from the node will be used most of the time.
>   	 */
> -	if (jNew->version > jOld->version) {
> -		/* since jNew is newer, we know jOld is not valid, so
> -		 * mark it with inode 0 and it will not be used
> +	struct jffs2_raw_dirent *jNew = get_node_mem(new->offset, NULL);
> +	struct jffs2_raw_dirent *jOld = get_node_mem(old->offset, NULL);
> +	int cmp;
> +	int ret;
> +
> +	if (jNew->pino != jOld->pino) {
> +		/* ascending sort by pino */
> +		ret = jNew->pino > jOld->pino;
> +	} else if (jNew->nsize != jOld->nsize) {
> +		/* pino is the same, so use ascending sort by nsize, so
> +		 * we don't do strncmp unless we really must.
>   		 */
> -		jOld->ino = 0;
> -		return 1;
> +		ret = jNew->nsize > jOld->nsize;
> +	} else {
> +		/* length is also the same, so use ascending sort by name
> +		 */
> +		cmp = strncmp((char *)jNew->name, (char *)jOld->name,
> +			jNew->nsize);
> +		if (cmp != 0) {
> +			ret = cmp > 0;
> +		} else {
> +			/* we have duplicate names in this directory,
> +			 * so use ascending sort by version
> +			 */
> +			ret = jNew->version > jOld->version;
> +		}
>   	}
> +	put_fl_mem(jNew, NULL);
> +	put_fl_mem(jOld, NULL);
>
> -	return 0;
> +	return ret;
>   }
>   #endif

Reviewed-by: Heiko Schocher <hs at denx.de>

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany