Skip to contentSkip to conversation

dont get radix-tree dirty tagging out of sync

PATCH In review Ryan Hope 3 participants 6 messages Aug 11, 2008 – Aug 12, 2008

lore
Share & tools
lore

Ryan Hope

author
#0 lore raw

PATCH 3/3 dont get radix-tree dirty tagging out of sync

This was item #14 on the todo list for reiser4 inclusion in mainline:

fs/reiser4/page_cache.c +4 -5
index fe71368..a662c25 100644
@@ -467,15 +467,14 @@ int reiser4_set_page_dirty_internal(struct page *page)
 	BUG_ON(mapping == NULL);
 
 	if (!TestSetPageDirty(page)) {
+		spin_lock_irq(&mapping->tree_lock);
 		if (mapping_cap_account_dirty(mapping))
 			inc_zone_page_state(page, NR_FILE_DIRTY);
-
+		radix_tree_tag_set(&mapping->page_tree,
+			page_index(page), PAGECACHE_TAG_DIRTY);
 		__mark_inode_dirty(mapping->host, I_DIRTY_PAGES);
+		spin_unlock_irq(&mapping->tree_lock);
 	}
-
-	/* znode must be dirty ? */
-	if (mapping->host == reiser4_get_super_fake(mapping->host->i_sb))
-		assert("", JF_ISSET(jprivate(page), JNODE_DIRTY));
 	return 0;
 }

-Ryan

#1 lore raw
Quoted 15 lines · Ryan Hope

This was item #14 on the todo list for reiser4 inclusion in mainline:

fs/reiser4/page_cache.c +2 -1
index fe71368..a662c25 100644
@@ -467,15 +467,14 @@ int reiser4_set_page_dirty_internal(struct page
 *page) BUG_ON(mapping == NULL);
 
  	if (!TestSetPageDirty(page)) {
+		spin_lock_irq(&mapping->tree_lock);
  		if (mapping_cap_account_dirty(mapping))
  			inc_zone_page_state(page, NR_FILE_DIRTY);
-
+		radix_tree_tag_set(&mapping->page_tree,

fs/reiser4/page_cache.c int reiser4_set_page_dirty_internal(struct page · line 479

Quoted 10 lines
fs/reiser4/page_cache.c +2 -4
+			page_index(page), PAGECACHE_TAG_DIRTY);
  		__mark_inode_dirty(mapping->host, I_DIRTY_PAGES);
+		spin_unlock_irq(&mapping->tree_lock);
  	}
-
-	/* znode must be dirty ? */
-	if (mapping->host == reiser4_get_super_fake(mapping->host->i_sb))
-		assert("", JF_ISSET(jprivate(page), JNODE_DIRTY));
  	return 0;
  }

Any reason why this can't use a generic function such as __set_page_dirty_nobuffers? There are accounting changes gone in there now which I suspetc may be wrong now in reiser4 (eg. task io accounting).

Actually every site that does a radix_tree_operation there should be reviewed to try to use core functoins.

author
#2 lore raw

I tried just using __set_page_dirty_nobuffers but that caused issues with ext3/ext4 (i can send a bug trace later if needed).

                                __inc_bdi_stat(mapping->backing_dev_info,
                                                BDI_RECLAIMABLE);
                                task_io_account_write(PAGE_CACHE_SIZE);

^^^ the above code in __set_page_dirty_nobuffers causes an issue with do_writepages in ext3/4 when I use something like the code below:

int reiser4_set_page_dirty_internal(struct page *page)

{
     return __set_page_dirty_nobuffers(page);
}
Quoted 35 lines · Nick Piggin
Quoted 15 lines · Ryan Hope

This was item #14 on the todo list for reiser4 inclusion in mainline:

fs/reiser4/page_cache.c +2 -1
index fe71368..a662c25 100644
@@ -467,15 +467,14 @@ int reiser4_set_page_dirty_internal(struct page
 *page) BUG_ON(mapping == NULL);
 
      if (!TestSetPageDirty(page)) {
+             spin_lock_irq(&mapping->tree_lock);
              if (mapping_cap_account_dirty(mapping))
                      inc_zone_page_state(page, NR_FILE_DIRTY);
-
+             radix_tree_tag_set(&mapping->page_tree,

fs/reiser4/page_cache.c int reiser4_set_page_dirty_internal(struct page · line 479

Quoted 10 lines
fs/reiser4/page_cache.c +2 -4
+                     page_index(page), PAGECACHE_TAG_DIRTY);
              __mark_inode_dirty(mapping->host, I_DIRTY_PAGES);
+             spin_unlock_irq(&mapping->tree_lock);
      }
-
-     /* znode must be dirty ? */
-     if (mapping->host == reiser4_get_super_fake(mapping->host->i_sb))
-             assert("", JF_ISSET(jprivate(page), JNODE_DIRTY));
      return 0;
  }

Any reason why this can't use a generic function such as __set_page_dirty_nobuffers? There are accounting changes gone in there now which I suspetc may be wrong now in reiser4 (eg. task io accounting).

Actually every site that does a radix_tree_operation there should be reviewed to try to use core functoins.

#3 lore raw
Quoted 7 lines · Ryan Hope

I tried just using __set_page_dirty_nobuffers but that caused issues with ext3/ext4 (i can send a bug trace later if needed).

                                __inc_bdi_stat(mapping->backing_dev_info,
                                                BDI_RECLAIMABLE);
                                task_io_account_write(PAGE_CACHE_SIZE);
Quoted 7 lines

^^^ the above code in __set_page_dirty_nobuffers causes an issue with do_writepages in ext3/4 when I use something like the code below:

int reiser4_set_page_dirty_internal(struct page *page)

{
     return __set_page_dirty_nobuffers(page);
}

Hmm, it causes issues in ext3/4 even though it doesn't change any code executed by ext3/4? Yes, if you could send a trace...

#5 lore raw
In reply to Nick Piggin
Quoted 4 lines · Nick Piggin
Quoted 2 lines · Ryan Hope

This was item #14 on the todo list for reiser4 inclusion in mainline:

No. This patch is a nonsense.
Where did you see radix-tree dirty tagging here?

Quoted 19 lines
Quoted 19 lines
fs/reiser4/page_cache.c +4 -3
index fe71368..a662c25 100644
@@ -467,15 +467,14 @@ int reiser4_set_page_dirty_internal(struct page
 *page) BUG_ON(mapping == NULL);
 
  	if (!TestSetPageDirty(page)) {
+		spin_lock_irq(&mapping->tree_lock);
  		if (mapping_cap_account_dirty(mapping))
  			inc_zone_page_state(page, NR_FILE_DIRTY);
-
+		radix_tree_tag_set(&mapping->page_tree,
+			page_index(page), PAGECACHE_TAG_DIRTY);
  		__mark_inode_dirty(mapping->host, I_DIRTY_PAGES);
+		spin_unlock_irq(&mapping->tree_lock);
  	}
-
-	/* znode must be dirty ? */
Quoted 8 lines

fs/reiser4/page_cache.c int reiser4_set_page_dirty_internal(struct page *page) · line 480

Quoted 5 lines
fs/reiser4/page_cache.c +0 -2
-	if (mapping->host == reiser4_get_super_fake(mapping->host->i_sb))
-		assert("", JF_ISSET(jprivate(page), JNODE_DIRTY));
  	return 0;
  }

Any reason why this can't use a generic function such as __set_page_dirty_nobuffers?

Currently reiser4 is working around "anonymous" pages dirtied outside of reiser4 context (e.g. via mmap), where some reiser4 specific work (jnode creation, capturing by an atom) can not be done.

Quoted 4 lines

There are accounting changes gone in
there now which I suspetc may be wrong now in reiser4 (eg. task io accounting).

Yes, this is definitely wrong. Thanks for pointing this out. Such poking around vfs internals should be fixed, otherwise we'll have permanent problems.

I have cc-ed Vladimir: maybe he has some hints. At least, I know that he looked at this problem..

Thanks,
Edward.

Quoted 4 lines

Actually every site that does a radix_tree_operation there should be reviewed to try to use core functoins.

author
#4 lore raw
In reply to Nick Piggin
Quoted 11 lines · Nick Piggin
Quoted 10 lines · Ryan Hope

I tried just using __set_page_dirty_nobuffers but that caused issues with ext3/ext4 (i can send a bug trace later if needed).

                                __inc_bdi_stat(mapping->backing_dev_info,
                                                BDI_RECLAIMABLE);
                                task_io_account_write(PAGE_CACHE_SIZE);

^^^ the above code in __set_page_dirty_nobuffers causes an issue with do_writepages in ext3/4 when I use something like the code below:

Quoted 8 lines
Quoted 4 lines

int reiser4_set_page_dirty_internal(struct page *page)

{
     return __set_page_dirty_nobuffers(page);
}

Hmm, it causes issues in ext3/4 even though it doesn't change any code executed by ext3/4? Yes, if you could send a trace...

Pid: 2620, comm: bonnie Not tainted 2.6.27-rc2-zen1 #1

 [<c026d729>] do_writepages+0x49/0x50
 [<c02dc830>] ext3_write_inode+0x0/0x40
 [<c02dc860>] ext3_write_inode+0x30/0x40
 [<c02ad3a2>] __writeback_single_inode+0x282/0x390
 [<c02ad9d4>] generic_sync_sb_inodes+0x304/0x3a0
 [<f9b9b234>] reiser4_sync_inodes+0x54/0x130 [reiser4]
 [<c02ad940>] generic_sync_sb_inodes+0x270/0x3a0
 [<c02adc64>] writeback_inodes+0x44/0xd0
 [<c026d0d0>] balance_dirty_pages_ratelimited_nr+0x230/0x370
 [<f9b79a5c>] reiser4_txn_end+0x4ec/0xae0 [reiser4]
 [<f9bc95ca>] balance_dirty_page_cluster+0x10a/0x1b0 [reiser4]
 [<f9bd17a0>] write_cryptcompress+0x610/0xdf0 [reiser4]
 [<f9bc2e76>] reiser4_write_careful+0xb6/0x880 [reiser4]
 [<c021c3ae>] update_curr+0x4e/0x70
 [<c02214c2>] task_tick_fair+0x32/0x90
 [<c023cf57>] hrtimer_forward+0xf7/0x150
 [<c0240523>] getnstimeofday+0x43/0xf0
 [<c0243885>] clockevents_program_event+0xa5/0x160
 [<c022f746>] run_timer_softirq+0x146/0x1c0
 [<c02449ca>] tick_program_event+0x3a/0x70
 [<c034a74c>] security_file_permission+0xc/0x10
 [<c028dcca>] rw_verify_area+0x4a/0xc0
 [<f9bc2dc0>] reiser4_write_careful+0x0/0x880 [reiser4]
 [<c028e1a0>] vfs_write+0xa0/0x140
 [<c028e311>] sys_write+0x41/0x80
 [<c020330d>] sysenter_do_call+0x12/0x25
 =======================