Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

Jörn Engel
2005-08-01T14:18:55Z

On Mon, 1 August 2005 16:02:32 +0200, Ferenc Havasi wrote:
> Jörn Engel wrote:
>
> >> D1(printk(KERN_DEBUG "Going to GC REF_PRISTINE node at 0x%08x\n", ref_offset(raw)));
> >>
> >>@@ -513,8 +514,9 @@ static int jffs2_garbage_collect_pristin
> >> /* Ask for a small amount of space (or the totlen if smaller) because we
> >> don't want to force wastage of the end of a block if splitting would
> >> work. */
> >>- ret = jffs2_reserve_space_gc(c, min_t(uint32_t, sizeof(struct jffs2_raw_inode) + JFFS2_MIN_DATA_LEN,
> >>- rawlen), &phys_ofs, &alloclen);
> >>+ ret = jffs2_reserve_space_gc(c, min_t(uint32_t, sizeof(struct jffs2_raw_inode) +
> >>+ JFFS2_MIN_DATA_LEN, rawlen), &phys_ofs, &alloclen, rawlen);
> >>+ /* this is not optimal yet */
> >
> >Can you add some more explanation to the comment?
> >
> OK, we will.
>
> The reason of "this is not optimal" is the following: for
> jffs2_reserve_space_gc() we have to specify an argument called sumsize.
> It is the summary size of the requested node. It needs it, because it is
> necessary to fit not only the node but also its summary representation
> into the erase block. In all other cases we know exactly the summary
> size of all nodes except in this case, so we made an upper-estimation here.

Makes sense.

> >> if (ret)
> >> return ret;
> >>
> >>@@ -594,7 +596,10 @@ static int jffs2_garbage_collect_pristin
> >> nraw->__totlen = rawlen;
> >> nraw->next_phys = NULL;
> >>
> >>- ret = jffs2_flash_write(c, phys_ofs, rawlen, &retlen, (char *)node);
> >>+ vecs[0].iov_base = (unsigned char *) node;
> >>+ vecs[0].iov_len = rawlen;
> >>+
> >>+ ret = jffs2_flash_writev(c, vecs, 1, phys_ofs, &retlen, 0);
> >
> >Why this change? If it's required, it might make sense to split it
> >out into a preparatory patch. But my gut feeling is that dropping
> >this part would be better.
> >
> Jffs2_flash_write() simply write directly the data onto the flash, and
> does not collect the summary information. Jffs2_flash_writev() does.

Then why can't you just re-implement jffs2_flash_write() using
Jffs2_flash_writev()? That way, you keep the strange one-element
array logic confined in a small and simple function. Here, we have an
already large and complicated one getting larger and more complicated.

Ok, maybe I'm missing something, so please tell me.

> >>+#ifdef CONFIG_JFFS2_SUMMARY
> >>+
> >>+/* Called with alloc sem _and_ erase_completion_lock */
> >>+static int jffs2_do_reserve_space_sum(struct jffs2_sb_info *c, uint32_t minsize, uint32_t *ofs, uint32_t *len, uint32_t sumsize)
> >>+{
> >>
> >[...]
> >
> >
> >>+#else
> >>+
> >>+/* Called with alloc sem _and_ erase_completion_lock */
> >>+static int jffs2_do_reserve_space_normal(struct jffs2_sb_info *c, uint32_t minsize, uint32_t *ofs, uint32_t *len, uint32_t sumsize)
> >>+{
> >
> >It doesn't take too much creativity to combine this and the above
> >#ifdef into a single one. That will still be ugly, but it's a step in
> >the right direction.
> >
> In the previous version it was combined, but we afraid of that it is
> hard to see what exectly happen. But we can tranform it back.

Hmm. Either way, I'm not happy yet. What I'd like to see is a patch
containing three types of changes:
o condition new code in fs/jffs2/summary.c,
o unconditional changes in existing source files and
o no-too-ugly #ifdefs in headers.

Nothing else. I guess the converged variant is a better basis to work
towards that goal, but I could be wrong. This one clearly needs more
thinking.

Jörn

--
Simplicity is prerequisite for reliability.
-- Edsger W. Dijkstra