[PATCH 1/3] f2fs: callers take care of the page from bio error

[PATCH 1/3] f2fs: callers take care of the page from bio error

[Jaegeuk Kim]

This patch changes for a caller to handle the page after its bio gets an error.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/checkpoint.c |  4 +++-
 fs/f2fs/data.c       | 27 +++++++++++++--------------
 fs/f2fs/node.c       | 21 ++++++++++-----------
 3 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 6032702..6fb696d 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -69,8 +69,10 @@ repeat:
 
 	fio.page = page;
 
-	if (f2fs_submit_page_bio(&fio))
+	if (f2fs_submit_page_bio(&fio)) {
+		f2fs_put_page(page, 1);
 		goto repeat;
+	}
 
 	lock_page(page);
 	if (unlikely(page->mapping != mapping)) {
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 57042bd..b621c08 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -156,7 +156,6 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
 
 	if (bio_add_page(bio, page, PAGE_CACHE_SIZE, 0) < PAGE_CACHE_SIZE) {
 		bio_put(bio);
-		f2fs_put_page(page, 1);
 		return -EFAULT;
 	}
 
@@ -292,15 +291,13 @@ struct page *get_read_data_page(struct inode *inode, pgoff_t index, int rw)
 
 	set_new_dnode(&dn, inode, NULL, NULL, 0);
 	err = get_dnode_of_data(&dn, index, LOOKUP_NODE);
-	if (err) {
-		f2fs_put_page(page, 1);
-		return ERR_PTR(err);
-	}
+	if (err)
+		goto put_err;
 	f2fs_put_dnode(&dn);
 
 	if (unlikely(dn.data_blkaddr == NULL_ADDR)) {
-		f2fs_put_page(page, 1);
-		return ERR_PTR(-ENOENT);
+		err = -ENOENT;
+		goto put_err;
 	}
 got_it:
 	if (PageUptodate(page)) {
@@ -325,8 +322,12 @@ got_it:
 	fio.page = page;
 	err = f2fs_submit_page_bio(&fio);
 	if (err)
-		return ERR_PTR(err);
+		goto put_err;
 	return page;
+
+put_err:
+	f2fs_put_page(page, 1);
+	return ERR_PTR(err);
 }
 
 struct page *find_data_page(struct inode *inode, pgoff_t index)
@@ -1329,7 +1330,8 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,
 {
 	struct inode *inode = mapping->host;
 	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
-	struct page *page, *ipage;
+	struct page *page = NULL;
+	struct page *ipage;
 	pgoff_t index = ((unsigned long long) pos) >> PAGE_CACHE_SHIFT;
 	struct dnode_of_data dn;
 	int err = 0;
@@ -1419,7 +1421,6 @@ put_next:
 
 		lock_page(page);
 		if (unlikely(!PageUptodate(page))) {
-			f2fs_put_page(page, 1);
 			err = -EIO;
 			goto fail;
 		}
@@ -1431,10 +1432,8 @@ put_next:
 		/* avoid symlink page */
 		if (f2fs_encrypted_inode(inode) && S_ISREG(inode->i_mode)) {
 			err = f2fs_decrypt_one(inode, page);
-			if (err) {
-				f2fs_put_page(page, 1);
+			if (err)
 				goto fail;
-			}
 		}
 	}
 out_update:
@@ -1447,8 +1446,8 @@ put_fail:
 	f2fs_put_dnode(&dn);
 unlock_fail:
 	f2fs_unlock_op(sbi);
-	f2fs_put_page(page, 1);
 fail:
+	f2fs_put_page(page, 1);
 	f2fs_write_failed(mapping, pos + len);
 	return err;
 }
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index a05eb35..7dd2b9d 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -991,8 +991,7 @@ fail:
 /*
  * Caller should do after getting the following values.
  * 0: f2fs_put_page(page, 0)
- * LOCKED_PAGE: f2fs_put_page(page, 1)
- * error: nothing
+ * LOCKED_PAGE or error: f2fs_put_page(page, 1)
  */
 static int read_node_page(struct page *page, int rw)
 {
@@ -1010,7 +1009,6 @@ static int read_node_page(struct page *page, int rw)
 
 	if (unlikely(ni.blk_addr == NULL_ADDR)) {
 		ClearPageUptodate(page);
-		f2fs_put_page(page, 1);
 		return -ENOENT;
 	}
 
@@ -1041,10 +1039,7 @@ void ra_node_page(struct f2fs_sb_info *sbi, nid_t nid)
 		return;
 
 	err = read_node_page(apage, READA);
-	if (err == 0)
-		f2fs_put_page(apage, 0);
-	else if (err == LOCKED_PAGE)
-		f2fs_put_page(apage, 1);
+	f2fs_put_page(apage, err ? 1 : 0);
 }
 
 struct page *get_node_page(struct f2fs_sb_info *sbi, pgoff_t nid)
@@ -1057,10 +1052,12 @@ repeat:
 		return ERR_PTR(-ENOMEM);
 
 	err = read_node_page(page, READ_SYNC);
-	if (err < 0)
+	if (err < 0) {
+		f2fs_put_page(page, 1);
 		return ERR_PTR(err);
-	else if (err != LOCKED_PAGE)
+	} else if (err != LOCKED_PAGE) {
 		lock_page(page);
+	}
 
 	if (unlikely(!PageUptodate(page) || nid != nid_of_node(page))) {
 		ClearPageUptodate(page);
@@ -1096,10 +1093,12 @@ repeat:
 		return ERR_PTR(-ENOMEM);
 
 	err = read_node_page(page, READ_SYNC);
-	if (err < 0)
+	if (err < 0) {
+		f2fs_put_page(page, 1);
 		return ERR_PTR(err);
-	else if (err == LOCKED_PAGE)
+	} else if (err == LOCKED_PAGE) {
 		goto page_hit;
+	}
 
 	blk_start_plug(&plug);
 
-- 
2.1.1

[PATCH 2/3] f2fs: handle error cases in move_encrypted_block

[Jaegeuk Kim]

This patch fixes some missing error handlers.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/gc.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 93aff5b..22007ad 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -559,27 +559,34 @@ static void move_encrypted_block(struct inode *inode, block_t bidx)
 	if (!fio.encrypted_page)
 		goto put_out;
 
-	f2fs_submit_page_bio(&fio);
+	err = f2fs_submit_page_bio(&fio);
+	if (err)
+		goto put_page_out;
+
+	/* write page */
+	lock_page(fio.encrypted_page);
+
+	if (unlikely(!PageUptodate(fio.encrypted_page)))
+		goto put_page_out;
+	if (unlikely(fio.encrypted_page->mapping != META_MAPPING(fio.sbi)))
+		goto put_page_out;
+
+	set_page_writeback(fio.encrypted_page);
 
 	/* allocate block address */
 	f2fs_wait_on_page_writeback(dn.node_page, NODE);
-
 	allocate_data_block(fio.sbi, NULL, fio.blk_addr,
 					&fio.blk_addr, &sum, CURSEG_COLD_DATA);
-	dn.data_blkaddr = fio.blk_addr;
-
-	/* write page */
-	lock_page(fio.encrypted_page);
-	set_page_writeback(fio.encrypted_page);
 	fio.rw = WRITE_SYNC;
 	f2fs_submit_page_mbio(&fio);
 
+	dn.data_blkaddr = fio.blk_addr;
 	set_data_blkaddr(&dn);
 	f2fs_update_extent_cache(&dn);
 	set_inode_flag(F2FS_I(inode), FI_APPEND_WRITE);
 	if (page->index == 0)
 		set_inode_flag(F2FS_I(inode), FI_FIRST_BLOCK_WRITTEN);
-
+put_page_out:
 	f2fs_put_page(fio.encrypted_page, 1);
 put_out:
 	f2fs_put_dnode(&dn);
-- 
2.1.1

[PATCH 3/3] f2fs: allow zeroed name length during find_dentry

[Jaegeuk Kim]

Since find_dentry doesn't lock its dentry page, it can traverse intermediate
bit positions consisting of a big dentry.
For these bit positions, this patch fills the intermediate name length fields
as zeros and skips them during look-up.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/dir.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index a34ebd8..71195f5 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -141,11 +141,15 @@ struct f2fs_dir_entry *find_target_dentry(struct f2fs_filename *fname,
 			*max_slots = max_len;
 		max_len = 0;
 
-		/* remain bug on condition */
-		if (unlikely(!de->name_len))
-			d->max = -1;
-
-		bit_pos += GET_DENTRY_SLOTS(le16_to_cpu(de->name_len));
+		/*
+		 * While a large dentry is adding, it can traverse its valid bit
+		 * positions, since find_entry does not lock its dentry page.
+		 * In that case, let's skip the bit position.
+		 */
+		if (de_name.len == 0)
+			bit_pos++;
+		else
+			bit_pos += GET_DENTRY_SLOTS(de_name.len);
 	}
 
 	de = NULL;
@@ -505,8 +509,12 @@ void f2fs_update_dentry(nid_t ino, umode_t mode, struct f2fs_dentry_ptr *d,
 	memcpy(d->filename[bit_pos], name->name, name->len);
 	de->ino = cpu_to_le32(ino);
 	set_de_type(de, mode);
-	for (i = 0; i < slots; i++)
+	for (i = 0; i < slots; i++) {
+		/* fill zeros for intermediate name_len slots */
+		if (i >= 1)
+			d->dentry[bit_pos + i].name_len = 0;
 		test_and_set_bit_le(bit_pos + i, (void *)d->bitmap);
+	}
 }
 
 /*
-- 
2.1.1

RE: [f2fs-dev] [PATCH 1/3] f2fs: callers take care of the page from bio error

[Chao Yu]

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Thursday, July 16, 2015 6:38 AM
> To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Cc: Jaegeuk Kim
> Subject: [f2fs-dev] [PATCH 1/3] f2fs: callers take care of the page from bio error
> 
> This patch changes for a caller to handle the page after its bio gets an error.

More clear!

> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

Reviewed-by: Chao Yu <chao2.yu@samsung.com>

RE: [f2fs-dev] [PATCH 2/3] f2fs: handle error cases in move_encrypted_block

[Chao Yu]

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Thursday, July 16, 2015 6:38 AM
> To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Cc: Jaegeuk Kim
> Subject: [f2fs-dev] [PATCH 2/3] f2fs: handle error cases in move_encrypted_block
> 
> This patch fixes some missing error handlers.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

Reviewed-by: Chao Yu <chao2.yu@samsung.com>

RE: [f2fs-dev] [PATCH 3/3] f2fs: allow zeroed name length during find_dentry

[Chao Yu]

Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Thursday, July 16, 2015 6:38 AM
> To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Cc: Jaegeuk Kim
> Subject: [f2fs-dev] [PATCH 3/3] f2fs: allow zeroed name length during find_dentry
> 
> Since find_dentry doesn't lock its dentry page, it can traverse intermediate
> bit positions consisting of a big dentry.
> For these bit positions, this patch fills the intermediate name length fields
> as zeros and skips them during look-up.

I didn't get it, AFAIK, dentry page will be accessed under protection of
i_mutex, if our inode operation has no bug, why this zeroed name_len can
exist?

Thanks,

> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/dir.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index a34ebd8..71195f5 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -141,11 +141,15 @@ struct f2fs_dir_entry *find_target_dentry(struct f2fs_filename *fname,
>  			*max_slots = max_len;
>  		max_len = 0;
> 
> -		/* remain bug on condition */
> -		if (unlikely(!de->name_len))
> -			d->max = -1;
> -
> -		bit_pos += GET_DENTRY_SLOTS(le16_to_cpu(de->name_len));
> +		/*
> +		 * While a large dentry is adding, it can traverse its valid bit
> +		 * positions, since find_entry does not lock its dentry page.
> +		 * In that case, let's skip the bit position.
> +		 */
> +		if (de_name.len == 0)
> +			bit_pos++;
> +		else
> +			bit_pos += GET_DENTRY_SLOTS(de_name.len);
>  	}
> 
>  	de = NULL;
> @@ -505,8 +509,12 @@ void f2fs_update_dentry(nid_t ino, umode_t mode, struct f2fs_dentry_ptr
> *d,
>  	memcpy(d->filename[bit_pos], name->name, name->len);
>  	de->ino = cpu_to_le32(ino);
>  	set_de_type(de, mode);
> -	for (i = 0; i < slots; i++)
> +	for (i = 0; i < slots; i++) {
> +		/* fill zeros for intermediate name_len slots */
> +		if (i >= 1)
> +			d->dentry[bit_pos + i].name_len = 0;
>  		test_and_set_bit_le(bit_pos + i, (void *)d->bitmap);
> +	}
>  }
> 
>  /*
> --
> 2.1.1
> 
> 
> ------------------------------------------------------------------------------
> Don't Limit Your Business. Reach for the Cloud.
> GigeNET's Cloud Solutions provide you with the tools and support that
> you need to offload your IT needs and focus on growing your business.
> Configured For All Businesses. Start Your Cloud Today.
> https://www.gigenetcloud.com/
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 3/3] f2fs: allow zeroed name length during find_dentry

[Jaegeuk Kim]

Hi Chao,

On Thu, Jul 16, 2015 at 06:03:46PM +0800, Chao Yu wrote:
> Hi Jaegeuk,
> 
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > Sent: Thursday, July 16, 2015 6:38 AM
> > To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> > linux-f2fs-devel@lists.sourceforge.net
> > Cc: Jaegeuk Kim
> > Subject: [f2fs-dev] [PATCH 3/3] f2fs: allow zeroed name length during find_dentry
> > 
> > Since find_dentry doesn't lock its dentry page, it can traverse intermediate
> > bit positions consisting of a big dentry.
> > For these bit positions, this patch fills the intermediate name length fields
> > as zeros and skips them during look-up.
> 
> I didn't get it, AFAIK, dentry page will be accessed under protection of
> i_mutex, if our inode operation has no bug, why this zeroed name_len can
> exist?

Indeed. I've checked that again, and i_mutex covers this.
I'll drop this patch, and let me keep an eye on any further bug report about
this. :)

Thanks,

> 
> Thanks,
> 
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  fs/f2fs/dir.c | 20 ++++++++++++++------
> >  1 file changed, 14 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> > index a34ebd8..71195f5 100644
> > --- a/fs/f2fs/dir.c
> > +++ b/fs/f2fs/dir.c
> > @@ -141,11 +141,15 @@ struct f2fs_dir_entry *find_target_dentry(struct f2fs_filename *fname,
> >  			*max_slots = max_len;
> >  		max_len = 0;
> > 
> > -		/* remain bug on condition */
> > -		if (unlikely(!de->name_len))
> > -			d->max = -1;
> > -
> > -		bit_pos += GET_DENTRY_SLOTS(le16_to_cpu(de->name_len));
> > +		/*
> > +		 * While a large dentry is adding, it can traverse its valid bit
> > +		 * positions, since find_entry does not lock its dentry page.
> > +		 * In that case, let's skip the bit position.
> > +		 */
> > +		if (de_name.len == 0)
> > +			bit_pos++;
> > +		else
> > +			bit_pos += GET_DENTRY_SLOTS(de_name.len);
> >  	}
> > 
> >  	de = NULL;
> > @@ -505,8 +509,12 @@ void f2fs_update_dentry(nid_t ino, umode_t mode, struct f2fs_dentry_ptr
> > *d,
> >  	memcpy(d->filename[bit_pos], name->name, name->len);
> >  	de->ino = cpu_to_le32(ino);
> >  	set_de_type(de, mode);
> > -	for (i = 0; i < slots; i++)
> > +	for (i = 0; i < slots; i++) {
> > +		/* fill zeros for intermediate name_len slots */
> > +		if (i >= 1)
> > +			d->dentry[bit_pos + i].name_len = 0;
> >  		test_and_set_bit_le(bit_pos + i, (void *)d->bitmap);
> > +	}
> >  }
> > 
> >  /*
> > --
> > 2.1.1
> > 
> > 
> > ------------------------------------------------------------------------------
> > Don't Limit Your Business. Reach for the Cloud.
> > GigeNET's Cloud Solutions provide you with the tools and support that
> > you need to offload your IT needs and focus on growing your business.
> > Configured For All Businesses. Start Your Cloud Today.
> > https://www.gigenetcloud.com/
> > _______________________________________________
> > Linux-f2fs-devel mailing list
> > Linux-f2fs-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel