Re: [PATCH 2/8] reiserfs: use kmap_local_folio() in _get_block_create_0()

On Sat, Dec 17, 2022 at 09:14:05AM -0800, Ira Weiny wrote:
> On Fri, Dec 16, 2022 at 08:53:41PM +0000, Matthew Wilcox (Oracle) wrote:
> > Switch from the deprecated kmap() to kmap_local_folio().  For the
> > kunmap_local(), I subtract off 'chars' to prevent the possibility that
> > p has wrapped into the next page.
> 
> Thanks for tackling this one.  I think the conversion is mostly safe because I
> don't see any reason the mapping is passed to another thread.
> 
> But comments like this make me leary:
> 
>          "But, this means the item might move if kmap schedules"
> 
> What does that mean?  That seems to imply there is something wrong with the
> base code separate from the kmapping.

I should probably have deleted that comment.  I'm pretty sure what it
refers to is that we don't hold a lock that prevents the item from
moving.  When ReiserFS was written, we didn't have CONFIG_PREEMPT, so 
if kmap() scheduled, that was a point at which the item could move.
I don't think I introduced any additional brokenness by converting
from kmap() to kmap_local().  Maybe I'm wrong and somebody who
understands ReiserFS can explain.

> To the patch, I think subtracting chars might be an issue.  If chars > offset
> and the loop takes the first 'if (done) break;' path then p will end up
> pointing at the previous page wouldn't it?

I thought about that and managed to convince myself that chars was
always < offset.  But now I'm not sure again.  Easiest way to fix
this is actually to move the p += chars before the if (done) break;.

I also need to rev this patch because it assumes that b_folio is a
single page.

diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c
index 008855ddb365..be13ce7a38e1 100644
--- a/fs/reiserfs/inode.c
+++ b/fs/reiserfs/inode.c
@@ -295,7 +295,6 @@ static int _get_block_create_0(struct inode *inode, sector_t block,
 	int ret;
 	int result;
 	int done = 0;
-	unsigned long offset;
 
 	/* prepare the key to look for the 'block'-th block of file */
 	make_cpu_key(&key, inode,
@@ -380,17 +379,16 @@ static int _get_block_create_0(struct inode *inode, sector_t block,
 		set_buffer_uptodate(bh_result);
 		goto finished;
 	}
-	/* read file tail into part of page */
-	offset = (cpu_key_k_offset(&key) - 1) & (PAGE_SIZE - 1);
 	copy_item_head(&tmp_ih, ih);
 
 	/*
 	 * we only want to kmap if we are reading the tail into the page.
 	 * this is not the common case, so we don't kmap until we are
-	 * sure we need to.  But, this means the item might move if
-	 * kmap schedules
+	 * sure we need to.
 	 */
-	p = kmap_local_folio(bh_result->b_folio, offset);
+	p = kmap_local_folio(bh_result->b_folio,
+			offset_in_folio(bh_result->b_folio,
+					cpu_key_k_offset(&key) - 1));
 	memset(p, 0, inode->i_sb->s_blocksize);
 	do {
 		if (!is_direct_le_ih(ih)) {
@@ -413,12 +411,11 @@ static int _get_block_create_0(struct inode *inode, sector_t block,
 			chars = ih_item_len(ih) - path.pos_in_item;
 		}
 		memcpy(p, ih_item_body(bh, ih) + path.pos_in_item, chars);
+		p += chars;
 
 		if (done)
 			break;
 
-		p += chars;
-
 		/*
 		 * we done, if read direct item is not the last item of
 		 * node FIXME: we could try to check right delimiting key