Re: [PATCH 3/3] reiser4: in our own sync writes, mark pages dirty before marking them writeback.

[Ivan Shapovalov <intelfx100@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2412 bytes --]

On 2015-10-12 at 11:07 +0200, Edward Shishkin wrote:
> On 10/10/2015 12:44 PM, Ivan Shapovalov wrote:
> [...]
> > I didn't check the code yet; I'll probably try with that assertion
> > converted into warning and split into two
> > (one for formatted and another for unformatted nodes), so that I
> > could check what type of nodes is responsible
> > for generating the final oops in set_page_writeback().
> 
> I suppose that oops happens on unformatted nodes, because
> all formatted nodes have the same host - a special "fake" inode
> with number 1, which gets i_wb at mount time (init_fake_inode()
> calls inode_attach_wb()).
> 
> Edward.

So, I've added three non-fatal checks right before
set_page_writeback():
- PageDirty(pg)
- JF_ISSET(cur, JNODE_DIRTY)
- pg->mapping->host->i_wb != NULL

In almost all times when either of the checks is triggered,
the first two checks are triggered at once (i. e. neither page nor
jnode is dirty, but i_wb is not NULL). This happens with both formatted
nodes (jnode_is_znode), unformatted nodes (jnode_is_unformatted) and
other nodes.

And, finally, there are very few warnings where jnode _is_ dirty, but
the page _isn't_. On the first such warning i_wb is also NULL. And,
just as you've suspected, this happens with an unformatted node.

I'll try to add a backtrace buffer into struct jnode and generate a
backtrace there on each jnode dirtying attempt... and then print it for
the problematic jnode.

-- 
Ivan Shapovalov / intelfx /

> 
> > 
> > > For unformatted nodes only code review
> > > can help. Normally, all modifications of unformatted nodes should
> > > look like the following:
> > > 
> > > struct page *page = jnode_page(node);
> > > lock_page(page);
> > > char *data = kmap(page);
> > > /* modifications are going here */
> > > kunmap(page);
> > > set_page_dirty_nobuffers(page); /* somebody forgets to do this */
> > > unlock_page(page);
> > > 
> > > Modifications of formatted nodes should look like the following:
> > > 
> > > longterm_lock_znode(node);
> > > zload(node);
> > > /* modifications are going here */
> > > zrelse(node);
> > > znode_make_dirty(node); /* somebody forgets to do this */
> > > longterm_unlock_znode();
> > > 
> > > Anyway, we can use your patch 3 as a temporal fixup.
> > The most persistent things are those conseived as the most
> > temporary
> > ones... ;)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 213 bytes --]