Patchwork Move CRC computation to separate function

login
register
mail settings
Submitter Joel Reardon
Date March 19, 2012, 10:46 p.m.
Message ID <alpine.DEB.2.00.1203191635030.22256@eristoteles.iwoars.net>
Download mbox | patch
Permalink /patch/147671/
State New
Headers show

Comments

Joel Reardon - March 19, 2012, 10:46 p.m.
This patch moves the computation of CRCs for data nodes from
within ubifs_prepare_node to a separate function ubifs_set_datanode_crc,
which takes a data node, and computes and sets the CRC. This is to avoid
duplication of the CRC computation code in other places where it may be
needed.


Signed-off-by: Joel Reardon <reardonj@inf.ethz.ch>

---
 fs/ubifs/io.c    |   16 +++++++++++++---
 fs/ubifs/ubifs.h |    1 +
 2 files changed, 14 insertions(+), 3 deletions(-)
Artem Bityutskiy - March 23, 2012, 2:09 p.m.
On Mon, 2012-03-19 at 23:46 +0100, Joel Reardon wrote:
>  /**
> + * ubifs_set_datanode_crc - writes the crc for a data node to the common
> + * header.
> + * @node: the data node
> + */
> +void ubifs_set_datanode_crc(void *node)
> +{
> +	struct ubifs_ch *ch = (struct ubifs_ch *) node;
> +	int len = le32_to_cpu(ch->len);
> +	ch->crc = cpu_to_le32(crc32(UBIFS_CRC32_INIT, node + 8, len - 8));
> +}

Will this be used by other code, outside of io.c? Because currently
there is only one user in the same file, which does not justify
introducing it and making non-static. If answers are "yes", then it is
better to make it static inline and put to misc.h instead, because it is
very small.

>  void ubifs_prepare_node(struct ubifs_info *c, void *node, int len, int pad)
>  {
> -	uint32_t crc;
>  	struct ubifs_ch *ch = node;
>  	unsigned long long sqnum = next_sqnum(c);
> 
> @@ -390,8 +401,7 @@ void ubifs_prepare_node(struct ubifs_info *c, void *node, int len, int pad)
>  	ch->group_type = UBIFS_NO_NODE_GROUP;
>  	ch->sqnum = cpu_to_le64(sqnum);
>  	ch->padding[0] = ch->padding[1] = 0;
> -	crc = crc32(UBIFS_CRC32_INIT, node + 8, len - 8);
> -	ch->crc = cpu_to_le32(crc);
> +	ubifs_set_datanode_crc(node);

But ubifs_prepare_node() is generic and works for any node type, not
just data nodes, which means that using 'datanode' in the name is not a
good idea.
Joel Reardon - March 23, 2012, 4:45 p.m.
When GCing a data node, it may be reencrypted as a means of organizing the
KSA and reducing the number of LEBs that need to be erased. A Long term
keys in one half for stable data, short term in the other.  During this
reencryption the CRC is recomputed. Also, after truncating it the last
chunk needs to be reencrypted too, or else the key can be used with the
old version to find the truncated part.

For the misc.h approach, it does not currently include ubifs-media.h, but
that is where UBIFS_CRC32_INIT is defined, so move it also to misc.h



On Fri, 23 Mar 2012, Artem Bityutskiy wrote:

> On Mon, 2012-03-19 at 23:46 +0100, Joel Reardon wrote:
> >  /**
> > + * ubifs_set_datanode_crc - writes the crc for a data node to the common
> > + * header.
> > + * @node: the data node
> > + */
> > +void ubifs_set_datanode_crc(void *node)
> > +{
> > +	struct ubifs_ch *ch = (struct ubifs_ch *) node;
> > +	int len = le32_to_cpu(ch->len);
> > +	ch->crc = cpu_to_le32(crc32(UBIFS_CRC32_INIT, node + 8, len - 8));
> > +}
>
> Will this be used by other code, outside of io.c? Because currently
> there is only one user in the same file, which does not justify
> introducing it and making non-static. If answers are "yes", then it is
> better to make it static inline and put to misc.h instead, because it is
> very small.
>
> >  void ubifs_prepare_node(struct ubifs_info *c, void *node, int len, int pad)
> >  {
> > -	uint32_t crc;
> >  	struct ubifs_ch *ch = node;
> >  	unsigned long long sqnum = next_sqnum(c);
> >
> > @@ -390,8 +401,7 @@ void ubifs_prepare_node(struct ubifs_info *c, void *node, int len, int pad)
> >  	ch->group_type = UBIFS_NO_NODE_GROUP;
> >  	ch->sqnum = cpu_to_le64(sqnum);
> >  	ch->padding[0] = ch->padding[1] = 0;
> > -	crc = crc32(UBIFS_CRC32_INIT, node + 8, len - 8);
> > -	ch->crc = cpu_to_le32(crc);
> > +	ubifs_set_datanode_crc(node);
>
> But ubifs_prepare_node() is generic and works for any node type, not
> just data nodes, which means that using 'datanode' in the name is not a
> good idea.
>
> --
> Best Regards,
> Artem Bityutskiy
>
Artem Bityutskiy - March 23, 2012, 4:51 p.m.
On Fri, 2012-03-23 at 17:45 +0100, Joel Reardon wrote:
> When GCing a data node, it may be reencrypted as a means of organizing the
> KSA and reducing the number of LEBs that need to be erased. A Long term
> keys in one half for stable data, short term in the other.  During this
> reencryption the CRC is recomputed. Also, after truncating it the last
> chunk needs to be reencrypted too, or else the key can be used with the
> old version to find the truncated part.

I guess this means there will be more users for this function.

> For the misc.h approach, it does not currently include ubifs-media.h, but
> that is where UBIFS_CRC32_INIT is defined, so move it also to misc.h

Just include it from misc.h.

Patch

diff --git a/fs/ubifs/io.c b/fs/ubifs/io.c
index 9228950..9d39b27 100644
--- a/fs/ubifs/io.c
+++ b/fs/ubifs/io.c
@@ -367,6 +367,18 @@  static unsigned long long next_sqnum(struct ubifs_info *c)
 }

 /**
+ * ubifs_set_datanode_crc - writes the crc for a data node to the common
+ * header.
+ * @node: the data node
+ */
+void ubifs_set_datanode_crc(void *node)
+{
+	struct ubifs_ch *ch = (struct ubifs_ch *) node;
+	int len = le32_to_cpu(ch->len);
+	ch->crc = cpu_to_le32(crc32(UBIFS_CRC32_INIT, node + 8, len - 8));
+}
+
+/**
  * ubifs_prepare_node - prepare node to be written to flash.
  * @c: UBIFS file-system description object
  * @node: the node to pad
@@ -379,7 +391,6 @@  static unsigned long long next_sqnum(struct ubifs_info *c)
  */
 void ubifs_prepare_node(struct ubifs_info *c, void *node, int len, int pad)
 {
-	uint32_t crc;
 	struct ubifs_ch *ch = node;
 	unsigned long long sqnum = next_sqnum(c);

@@ -390,8 +401,7 @@  void ubifs_prepare_node(struct ubifs_info *c, void *node, int len, int pad)
 	ch->group_type = UBIFS_NO_NODE_GROUP;
 	ch->sqnum = cpu_to_le64(sqnum);
 	ch->padding[0] = ch->padding[1] = 0;
-	crc = crc32(UBIFS_CRC32_INIT, node + 8, len - 8);
-	ch->crc = cpu_to_le32(crc);
+	ubifs_set_datanode_crc(node);

 	if (pad) {
 		len = ALIGN(len, 8);
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index 7c343e1..bfd0798 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -1486,6 +1486,7 @@  int ubifs_write_node(struct ubifs_info *c, void *node, int len, int lnum,
 		     int offs, int dtype);
 int ubifs_check_node(const struct ubifs_info *c, const void *buf, int lnum,
 		     int offs, int quiet, int must_chk_crc);
+void ubifs_set_datanode_crc(void *node);
 void ubifs_prepare_node(struct ubifs_info *c, void *buf, int len, int pad);
 void ubifs_prep_grp_node(struct ubifs_info *c, void *node, int len, int last);
 int ubifs_io_init(struct ubifs_info *c);