Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1297724 > unrolled thread

[PATCH 3/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin

Started byJaegeuk Kim <jaegeuk@kernel.org>
First post2015-12-24 03:20 +0100
Last post2015-12-25 02:50 +0100
Articles 6 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin Jaegeuk Kim <jaegeuk@kernel.org> - 2015-12-24 03:20 +0100
    RE: [f2fs-dev] [PATCH 3/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin Chao Yu <chao2.yu@samsung.com> - 2015-12-24 07:00 +0100
      Re: [f2fs-dev] [PATCH 3/4] f2fs: avoid f2fs_lock_op in  f2fs_write_begin Jaegeuk Kim <jaegeuk@kernel.org> - 2015-12-24 21:40 +0100
        Re: [f2fs-dev] [PATCH 3/4] f2fs: avoid f2fs_lock_op in  f2fs_write_begin Jaegeuk Kim <jaegeuk@kernel.org> - 2015-12-24 23:00 +0100
          RE: [f2fs-dev] [PATCH 3/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin Chao Yu <chao2.yu@samsung.com> - 2015-12-25 02:20 +0100
            Re: [f2fs-dev] [PATCH 3/4] f2fs: avoid f2fs_lock_op in  f2fs_write_begin Jaegeuk Kim <jaegeuk@kernel.org> - 2015-12-25 02:50 +0100

#1297724 — [PATCH 3/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin

FromJaegeuk Kim <jaegeuk@kernel.org>
Date2015-12-24 03:20 +0100
Subject[PATCH 3/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin
Message-ID<qJ3Fv-Na-9@gated-at.bofh.it>
If f2fs_write_begin is to update data, we can bypass calling f2fs_lock_op() in
order to avoid the checkpoint latency in the write syscall.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/data.c | 43 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 34 insertions(+), 9 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 49092da..d53cf4f 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1418,10 +1418,16 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
 	pgoff_t index = page->index;
 	struct dnode_of_data dn;
 	struct page *ipage;
+	bool locked = false;
+	struct extent_info ei;
 	int err = 0;
 
-	f2fs_lock_op(sbi);
-
+	if (f2fs_has_inline_data(inode) ||
+			(pos & PAGE_CACHE_MASK) >= i_size_read(inode)) {
+		f2fs_lock_op(sbi);
+		locked = true;
+	}
+restart:
 	/* check inline_data */
 	ipage = get_node_page(sbi, inode->i_ino);
 	if (IS_ERR(ipage)) {
@@ -1436,22 +1442,41 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
 			read_inline_data(page, ipage);
 			set_inode_flag(F2FS_I(inode), FI_DATA_EXIST);
 			sync_inode_page(&dn);
-			goto done;
 		} else {
 			err = f2fs_convert_inline_page(&dn, page);
 			if (err)
-				goto err_out;
+				goto out;
+			err = f2fs_get_block(&dn, index);
+		}
+	} else if (locked) {
+		err = f2fs_get_block(&dn, index);
+	} else {
+		if (f2fs_lookup_extent_cache(inode, index, &ei)) {
+			dn.data_blkaddr = ei.blk + index - ei.fofs;
+		} else {
+			bool restart = false;
+
+			/* hole case */
+			err = get_dnode_of_data(&dn, index, LOOKUP_NODE);
+			if (err || (!err && dn.data_blkaddr == NULL_ADDR))
+				restart = true;
+			if (restart) {
+				f2fs_put_dnode(&dn);
+				f2fs_lock_op(sbi);
+				locked = true;
+				goto restart;
+			}
 		}
 	}
-	err = f2fs_get_block(&dn, index);
-done:
+
 	/* convert_inline_page can make node_changed */
 	*blk_addr = dn.data_blkaddr;
 	*node_changed = dn.node_changed;
-err_out:
+out:
 	f2fs_put_dnode(&dn);
 unlock_out:
-	f2fs_unlock_op(sbi);
+	if (locked)
+		f2fs_unlock_op(sbi);
 	return err;
 }
 
@@ -1488,7 +1513,7 @@ repeat:
 
 	*pagep = page;
 
-	err = prepare_write_begin(sbi, page, pos + len,
+	err = prepare_write_begin(sbi, page, pos,  len,
 					&blkaddr, &need_balance);
 	if (err)
 		goto fail;
-- 
2.5.4 (Apple Git-61)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1297760 — RE: [f2fs-dev] [PATCH 3/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin

FromChao Yu <chao2.yu@samsung.com>
Date2015-12-24 07:00 +0100
SubjectRE: [f2fs-dev] [PATCH 3/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin
Message-ID<qJ76p-2O5-1@gated-at.bofh.it>
In reply to#1297724
Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Thursday, December 24, 2015 10:15 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/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin
> 
> If f2fs_write_begin is to update data, we can bypass calling f2fs_lock_op() in
> order to avoid the checkpoint latency in the write syscall.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/data.c | 43 ++++++++++++++++++++++++++++++++++---------
>  1 file changed, 34 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 49092da..d53cf4f 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1418,10 +1418,16 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
>  	pgoff_t index = page->index;
>  	struct dnode_of_data dn;
>  	struct page *ipage;
> +	bool locked = false;
> +	struct extent_info ei;
>  	int err = 0;
> 
> -	f2fs_lock_op(sbi);
> -
> +	if (f2fs_has_inline_data(inode) ||
> +			(pos & PAGE_CACHE_MASK) >= i_size_read(inode)) {
> +		f2fs_lock_op(sbi);
> +		locked = true;
> +	}
> +restart:
>  	/* check inline_data */
>  	ipage = get_node_page(sbi, inode->i_ino);
>  	if (IS_ERR(ipage)) {
> @@ -1436,22 +1442,41 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
>  			read_inline_data(page, ipage);
>  			set_inode_flag(F2FS_I(inode), FI_DATA_EXIST);
>  			sync_inode_page(&dn);
> -			goto done;
>  		} else {
>  			err = f2fs_convert_inline_page(&dn, page);
>  			if (err)
> -				goto err_out;
> +				goto out;
> +			err = f2fs_get_block(&dn, index);

Seems after f2fs_convert_inline_page, data_blkaddr and node_changed should
have been set, so f2fs_get_block could be removed here.

> +		}
> +	} else if (locked) {
> +		err = f2fs_get_block(&dn, index);
> +	} else {
> +		if (f2fs_lookup_extent_cache(inode, index, &ei)) {
> +			dn.data_blkaddr = ei.blk + index - ei.fofs;
> +		} else {
> +			bool restart = false;
> +
> +			/* hole case */
> +			err = get_dnode_of_data(&dn, index, LOOKUP_NODE);

Better to handle error case like -ENOMEM or -EIO.

Thanks,

> +			if (err || (!err && dn.data_blkaddr == NULL_ADDR))
> +				restart = true;
> +			if (restart) {
> +				f2fs_put_dnode(&dn);
> +				f2fs_lock_op(sbi);
> +				locked = true;
> +				goto restart;
> +			}
>  		}
>  	}
> -	err = f2fs_get_block(&dn, index);
> -done:
> +
>  	/* convert_inline_page can make node_changed */
>  	*blk_addr = dn.data_blkaddr;
>  	*node_changed = dn.node_changed;
> -err_out:
> +out:
>  	f2fs_put_dnode(&dn);
>  unlock_out:
> -	f2fs_unlock_op(sbi);
> +	if (locked)
> +		f2fs_unlock_op(sbi);
>  	return err;
>  }
> 
> @@ -1488,7 +1513,7 @@ repeat:
> 
>  	*pagep = page;
> 
> -	err = prepare_write_begin(sbi, page, pos + len,
> +	err = prepare_write_begin(sbi, page, pos,  len,
>  					&blkaddr, &need_balance);
>  	if (err)
>  		goto fail;
> --
> 2.5.4 (Apple Git-61)
> 
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1298041 — Re: [f2fs-dev] [PATCH 3/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin

FromJaegeuk Kim <jaegeuk@kernel.org>
Date2015-12-24 21:40 +0100
SubjectRe: [f2fs-dev] [PATCH 3/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin
Message-ID<qJkQ1-2Td-1@gated-at.bofh.it>
In reply to#1297760
Hi Chao,

On Thu, Dec 24, 2015 at 01:50:32PM +0800, Chao Yu wrote:
> Hi Jaegeuk,
> 
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > Sent: Thursday, December 24, 2015 10:15 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/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin
> > 
> > If f2fs_write_begin is to update data, we can bypass calling f2fs_lock_op() in
> > order to avoid the checkpoint latency in the write syscall.
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  fs/f2fs/data.c | 43 ++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 34 insertions(+), 9 deletions(-)
> > 
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index 49092da..d53cf4f 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -1418,10 +1418,16 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
> >  	pgoff_t index = page->index;
> >  	struct dnode_of_data dn;
> >  	struct page *ipage;
> > +	bool locked = false;
> > +	struct extent_info ei;
> >  	int err = 0;
> > 
> > -	f2fs_lock_op(sbi);
> > -
> > +	if (f2fs_has_inline_data(inode) ||
> > +			(pos & PAGE_CACHE_MASK) >= i_size_read(inode)) {
> > +		f2fs_lock_op(sbi);
> > +		locked = true;
> > +	}
> > +restart:
> >  	/* check inline_data */
> >  	ipage = get_node_page(sbi, inode->i_ino);
> >  	if (IS_ERR(ipage)) {
> > @@ -1436,22 +1442,41 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
> >  			read_inline_data(page, ipage);
> >  			set_inode_flag(F2FS_I(inode), FI_DATA_EXIST);
> >  			sync_inode_page(&dn);
> > -			goto done;
> >  		} else {
> >  			err = f2fs_convert_inline_page(&dn, page);
> >  			if (err)
> > -				goto err_out;
> > +				goto out;
> > +			err = f2fs_get_block(&dn, index);
> 
> Seems after f2fs_convert_inline_page, data_blkaddr and node_changed should
> have been set, so f2fs_get_block could be removed here.

I don't think so. The inline_data treats with 0'th index only.
In order to handle non-zero index, we should do f2fs_get_block.
Oh, if you meant zero index case only, we can do that like this.

			if (index)
				err = f2fs_get_block();

> 
> > +		}
> > +	} else if (locked) {
> > +		err = f2fs_get_block(&dn, index);
> > +	} else {
> > +		if (f2fs_lookup_extent_cache(inode, index, &ei)) {
> > +			dn.data_blkaddr = ei.blk + index - ei.fofs;
> > +		} else {
> > +			bool restart = false;
> > +
> > +			/* hole case */
> > +			err = get_dnode_of_data(&dn, index, LOOKUP_NODE);
> 
> Better to handle error case like -ENOMEM or -EIO.

I think we can just give another chance at this moment and let f2fs_get_block()
handle that in the next round.

Thanks,

> 
> Thanks,
> 
> > +			if (err || (!err && dn.data_blkaddr == NULL_ADDR))
> > +				restart = true;
> > +			if (restart) {
> > +				f2fs_put_dnode(&dn);
> > +				f2fs_lock_op(sbi);
> > +				locked = true;
> > +				goto restart;
> > +			}
> >  		}
> >  	}
> > -	err = f2fs_get_block(&dn, index);
> > -done:
> > +
> >  	/* convert_inline_page can make node_changed */
> >  	*blk_addr = dn.data_blkaddr;
> >  	*node_changed = dn.node_changed;
> > -err_out:
> > +out:
> >  	f2fs_put_dnode(&dn);
> >  unlock_out:
> > -	f2fs_unlock_op(sbi);
> > +	if (locked)
> > +		f2fs_unlock_op(sbi);
> >  	return err;
> >  }
> > 
> > @@ -1488,7 +1513,7 @@ repeat:
> > 
> >  	*pagep = page;
> > 
> > -	err = prepare_write_begin(sbi, page, pos + len,
> > +	err = prepare_write_begin(sbi, page, pos,  len,
> >  					&blkaddr, &need_balance);
> >  	if (err)
> >  		goto fail;
> > --
> > 2.5.4 (Apple Git-61)
> > 
> > 
> > ------------------------------------------------------------------------------
> > _______________________________________________
> > Linux-f2fs-devel mailing list
> > Linux-f2fs-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1298055 — Re: [f2fs-dev] [PATCH 3/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin

FromJaegeuk Kim <jaegeuk@kernel.org>
Date2015-12-24 23:00 +0100
SubjectRe: [f2fs-dev] [PATCH 3/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin
Message-ID<qJm5s-3xU-3@gated-at.bofh.it>
In reply to#1298041
Hi Chao,

On Thu, Dec 24, 2015 at 12:33:50PM -0800, Jaegeuk Kim wrote:
> Hi Chao,
> 
> On Thu, Dec 24, 2015 at 01:50:32PM +0800, Chao Yu wrote:
> > Hi Jaegeuk,
> > 
> > > -----Original Message-----
> > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > > Sent: Thursday, December 24, 2015 10:15 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/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin
> > > 
> > > If f2fs_write_begin is to update data, we can bypass calling f2fs_lock_op() in
> > > order to avoid the checkpoint latency in the write syscall.
> > > 
> > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > > ---
> > >  fs/f2fs/data.c | 43 ++++++++++++++++++++++++++++++++++---------
> > >  1 file changed, 34 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > > index 49092da..d53cf4f 100644
> > > --- a/fs/f2fs/data.c
> > > +++ b/fs/f2fs/data.c
> > > @@ -1418,10 +1418,16 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
> > >  	pgoff_t index = page->index;
> > >  	struct dnode_of_data dn;
> > >  	struct page *ipage;
> > > +	bool locked = false;
> > > +	struct extent_info ei;
> > >  	int err = 0;
> > > 
> > > -	f2fs_lock_op(sbi);
> > > -
> > > +	if (f2fs_has_inline_data(inode) ||
> > > +			(pos & PAGE_CACHE_MASK) >= i_size_read(inode)) {
> > > +		f2fs_lock_op(sbi);
> > > +		locked = true;
> > > +	}
> > > +restart:
> > >  	/* check inline_data */
> > >  	ipage = get_node_page(sbi, inode->i_ino);
> > >  	if (IS_ERR(ipage)) {
> > > @@ -1436,22 +1442,41 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
> > >  			read_inline_data(page, ipage);
> > >  			set_inode_flag(F2FS_I(inode), FI_DATA_EXIST);
> > >  			sync_inode_page(&dn);
> > > -			goto done;
> > >  		} else {
> > >  			err = f2fs_convert_inline_page(&dn, page);
> > >  			if (err)
> > > -				goto err_out;
> > > +				goto out;
> > > +			err = f2fs_get_block(&dn, index);
> > 
> > Seems after f2fs_convert_inline_page, data_blkaddr and node_changed should
> > have been set, so f2fs_get_block could be removed here.
> 
> I don't think so. The inline_data treats with 0'th index only.
> In order to handle non-zero index, we should do f2fs_get_block.
> Oh, if you meant zero index case only, we can do that like this.
> 
> 			if (index)
> 				err = f2fs_get_block();

Actually, we need
			if (index || !dn.data_blkaddr)
				err = f2fs_get_block();

Since f2fs_convert_inline_page can return right away, if there is
no data in inline_data space.

Thanks,

> 
> > 
> > > +		}
> > > +	} else if (locked) {
> > > +		err = f2fs_get_block(&dn, index);
> > > +	} else {
> > > +		if (f2fs_lookup_extent_cache(inode, index, &ei)) {
> > > +			dn.data_blkaddr = ei.blk + index - ei.fofs;
> > > +		} else {
> > > +			bool restart = false;
> > > +
> > > +			/* hole case */
> > > +			err = get_dnode_of_data(&dn, index, LOOKUP_NODE);
> > 
> > Better to handle error case like -ENOMEM or -EIO.
> 
> I think we can just give another chance at this moment and let f2fs_get_block()
> handle that in the next round.
> 
> Thanks,
> 
> > 
> > Thanks,
> > 
> > > +			if (err || (!err && dn.data_blkaddr == NULL_ADDR))
> > > +				restart = true;
> > > +			if (restart) {
> > > +				f2fs_put_dnode(&dn);
> > > +				f2fs_lock_op(sbi);
> > > +				locked = true;
> > > +				goto restart;
> > > +			}
> > >  		}
> > >  	}
> > > -	err = f2fs_get_block(&dn, index);
> > > -done:
> > > +
> > >  	/* convert_inline_page can make node_changed */
> > >  	*blk_addr = dn.data_blkaddr;
> > >  	*node_changed = dn.node_changed;
> > > -err_out:
> > > +out:
> > >  	f2fs_put_dnode(&dn);
> > >  unlock_out:
> > > -	f2fs_unlock_op(sbi);
> > > +	if (locked)
> > > +		f2fs_unlock_op(sbi);
> > >  	return err;
> > >  }
> > > 
> > > @@ -1488,7 +1513,7 @@ repeat:
> > > 
> > >  	*pagep = page;
> > > 
> > > -	err = prepare_write_begin(sbi, page, pos + len,
> > > +	err = prepare_write_begin(sbi, page, pos,  len,
> > >  					&blkaddr, &need_balance);
> > >  	if (err)
> > >  		goto fail;
> > > --
> > > 2.5.4 (Apple Git-61)
> > > 
> > > 
> > > ------------------------------------------------------------------------------
> > > _______________________________________________
> > > Linux-f2fs-devel mailing list
> > > Linux-f2fs-devel@lists.sourceforge.net
> > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1298071 — RE: [f2fs-dev] [PATCH 3/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin

FromChao Yu <chao2.yu@samsung.com>
Date2015-12-25 02:20 +0100
SubjectRE: [f2fs-dev] [PATCH 3/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin
Message-ID<qJpcZ-5x7-1@gated-at.bofh.it>
In reply to#1298055
Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Friday, December 25, 2015 5:52 AM
> To: Chao Yu
> Cc: linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Subject: Re: [f2fs-dev] [PATCH 3/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin
> 
> Hi Chao,
> 
> On Thu, Dec 24, 2015 at 12:33:50PM -0800, Jaegeuk Kim wrote:
> > Hi Chao,
> >
> > On Thu, Dec 24, 2015 at 01:50:32PM +0800, Chao Yu wrote:
> > > Hi Jaegeuk,
> > >
> > > > -----Original Message-----
> > > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > > > Sent: Thursday, December 24, 2015 10:15 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/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin
> > > >
> > > > If f2fs_write_begin is to update data, we can bypass calling f2fs_lock_op() in
> > > > order to avoid the checkpoint latency in the write syscall.
> > > >
> > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > > > ---
> > > >  fs/f2fs/data.c | 43 ++++++++++++++++++++++++++++++++++---------
> > > >  1 file changed, 34 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > > > index 49092da..d53cf4f 100644
> > > > --- a/fs/f2fs/data.c
> > > > +++ b/fs/f2fs/data.c
> > > > @@ -1418,10 +1418,16 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
> > > >  	pgoff_t index = page->index;
> > > >  	struct dnode_of_data dn;
> > > >  	struct page *ipage;
> > > > +	bool locked = false;
> > > > +	struct extent_info ei;
> > > >  	int err = 0;
> > > >
> > > > -	f2fs_lock_op(sbi);
> > > > -
> > > > +	if (f2fs_has_inline_data(inode) ||
> > > > +			(pos & PAGE_CACHE_MASK) >= i_size_read(inode)) {
> > > > +		f2fs_lock_op(sbi);
> > > > +		locked = true;
> > > > +	}
> > > > +restart:
> > > >  	/* check inline_data */
> > > >  	ipage = get_node_page(sbi, inode->i_ino);
> > > >  	if (IS_ERR(ipage)) {
> > > > @@ -1436,22 +1442,41 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
> > > >  			read_inline_data(page, ipage);
> > > >  			set_inode_flag(F2FS_I(inode), FI_DATA_EXIST);
> > > >  			sync_inode_page(&dn);
> > > > -			goto done;
> > > >  		} else {
> > > >  			err = f2fs_convert_inline_page(&dn, page);
> > > >  			if (err)
> > > > -				goto err_out;
> > > > +				goto out;
> > > > +			err = f2fs_get_block(&dn, index);
> > >
> > > Seems after f2fs_convert_inline_page, data_blkaddr and node_changed should
> > > have been set, so f2fs_get_block could be removed here.
> >
> > I don't think so. The inline_data treats with 0'th index only.
> > In order to handle non-zero index, we should do f2fs_get_block.

non-zero index page should have been handled before grab_cache_page_write_begin.
We won't encounter such case here.

> > Oh, if you meant zero index case only, we can do that like this.
> >
> > 			if (index)
> > 				err = f2fs_get_block();
> 
> Actually, we need
> 			if (index || !dn.data_blkaddr)
> 				err = f2fs_get_block();
> 
> Since f2fs_convert_inline_page can return right away, if there is
> no data in inline_data space.

Yes, that's the case we should handle. :)

Thanks,

> 
> Thanks,
> 
> >
> > >
> > > > +		}
> > > > +	} else if (locked) {
> > > > +		err = f2fs_get_block(&dn, index);
> > > > +	} else {
> > > > +		if (f2fs_lookup_extent_cache(inode, index, &ei)) {
> > > > +			dn.data_blkaddr = ei.blk + index - ei.fofs;
> > > > +		} else {
> > > > +			bool restart = false;
> > > > +
> > > > +			/* hole case */
> > > > +			err = get_dnode_of_data(&dn, index, LOOKUP_NODE);
> > >
> > > Better to handle error case like -ENOMEM or -EIO.
> >
> > I think we can just give another chance at this moment and let f2fs_get_block()
> > handle that in the next round.
> >
> > Thanks,
> >
> > >
> > > Thanks,
> > >
> > > > +			if (err || (!err && dn.data_blkaddr == NULL_ADDR))
> > > > +				restart = true;
> > > > +			if (restart) {
> > > > +				f2fs_put_dnode(&dn);
> > > > +				f2fs_lock_op(sbi);
> > > > +				locked = true;
> > > > +				goto restart;
> > > > +			}
> > > >  		}
> > > >  	}
> > > > -	err = f2fs_get_block(&dn, index);
> > > > -done:
> > > > +
> > > >  	/* convert_inline_page can make node_changed */
> > > >  	*blk_addr = dn.data_blkaddr;
> > > >  	*node_changed = dn.node_changed;
> > > > -err_out:
> > > > +out:
> > > >  	f2fs_put_dnode(&dn);
> > > >  unlock_out:
> > > > -	f2fs_unlock_op(sbi);
> > > > +	if (locked)
> > > > +		f2fs_unlock_op(sbi);
> > > >  	return err;
> > > >  }
> > > >
> > > > @@ -1488,7 +1513,7 @@ repeat:
> > > >
> > > >  	*pagep = page;
> > > >
> > > > -	err = prepare_write_begin(sbi, page, pos + len,
> > > > +	err = prepare_write_begin(sbi, page, pos,  len,
> > > >  					&blkaddr, &need_balance);
> > > >  	if (err)
> > > >  		goto fail;
> > > > --
> > > > 2.5.4 (Apple Git-61)
> > > >
> > > >
> > > > ------------------------------------------------------------------------------
> > > > _______________________________________________
> > > > Linux-f2fs-devel mailing list
> > > > Linux-f2fs-devel@lists.sourceforge.net
> > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> >
> > ------------------------------------------------------------------------------
> > _______________________________________________
> > Linux-f2fs-devel mailing list
> > Linux-f2fs-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1298076 — Re: [f2fs-dev] [PATCH 3/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin

FromJaegeuk Kim <jaegeuk@kernel.org>
Date2015-12-25 02:50 +0100
SubjectRe: [f2fs-dev] [PATCH 3/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin
Message-ID<qJpG1-5Gz-5@gated-at.bofh.it>
In reply to#1298071
Hi Chao,

On Fri, Dec 25, 2015 at 09:18:06AM +0800, Chao Yu wrote:
> Hi Jaegeuk,
> 
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > Sent: Friday, December 25, 2015 5:52 AM
> > To: Chao Yu
> > Cc: linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org;
> > linux-f2fs-devel@lists.sourceforge.net
> > Subject: Re: [f2fs-dev] [PATCH 3/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin
> > 
> > Hi Chao,
> > 
> > On Thu, Dec 24, 2015 at 12:33:50PM -0800, Jaegeuk Kim wrote:
> > > Hi Chao,
> > >
> > > On Thu, Dec 24, 2015 at 01:50:32PM +0800, Chao Yu wrote:
> > > > Hi Jaegeuk,
> > > >
> > > > > -----Original Message-----
> > > > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > > > > Sent: Thursday, December 24, 2015 10:15 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/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin
> > > > >
> > > > > If f2fs_write_begin is to update data, we can bypass calling f2fs_lock_op() in
> > > > > order to avoid the checkpoint latency in the write syscall.
> > > > >
> > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > > > > ---
> > > > >  fs/f2fs/data.c | 43 ++++++++++++++++++++++++++++++++++---------
> > > > >  1 file changed, 34 insertions(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > > > > index 49092da..d53cf4f 100644
> > > > > --- a/fs/f2fs/data.c
> > > > > +++ b/fs/f2fs/data.c
> > > > > @@ -1418,10 +1418,16 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
> > > > >  	pgoff_t index = page->index;
> > > > >  	struct dnode_of_data dn;
> > > > >  	struct page *ipage;
> > > > > +	bool locked = false;
> > > > > +	struct extent_info ei;
> > > > >  	int err = 0;
> > > > >
> > > > > -	f2fs_lock_op(sbi);
> > > > > -
> > > > > +	if (f2fs_has_inline_data(inode) ||
> > > > > +			(pos & PAGE_CACHE_MASK) >= i_size_read(inode)) {
> > > > > +		f2fs_lock_op(sbi);
> > > > > +		locked = true;
> > > > > +	}
> > > > > +restart:
> > > > >  	/* check inline_data */
> > > > >  	ipage = get_node_page(sbi, inode->i_ino);
> > > > >  	if (IS_ERR(ipage)) {
> > > > > @@ -1436,22 +1442,41 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
> > > > >  			read_inline_data(page, ipage);
> > > > >  			set_inode_flag(F2FS_I(inode), FI_DATA_EXIST);
> > > > >  			sync_inode_page(&dn);
> > > > > -			goto done;
> > > > >  		} else {
> > > > >  			err = f2fs_convert_inline_page(&dn, page);
> > > > >  			if (err)
> > > > > -				goto err_out;
> > > > > +				goto out;
> > > > > +			err = f2fs_get_block(&dn, index);
> > > >
> > > > Seems after f2fs_convert_inline_page, data_blkaddr and node_changed should
> > > > have been set, so f2fs_get_block could be removed here.
> > >
> > > I don't think so. The inline_data treats with 0'th index only.
> > > In order to handle non-zero index, we should do f2fs_get_block.
> 
> non-zero index page should have been handled before grab_cache_page_write_begin.
> We won't encounter such case here.

Alright.
So, let me go with:
		if (dn.data_blkaddr == NULL_ADDR)
			err = f2fs_get_block();

Thanks,

> 
> > > Oh, if you meant zero index case only, we can do that like this.
> > >
> > > 			if (index)
> > > 				err = f2fs_get_block();
> > 
> > Actually, we need
> > 			if (index || !dn.data_blkaddr)
> > 				err = f2fs_get_block();
> > 
> > Since f2fs_convert_inline_page can return right away, if there is
> > no data in inline_data space.
> 
> Yes, that's the case we should handle. :)
> 
> Thanks,
> 
> > 
> > Thanks,
> > 
> > >
> > > >
> > > > > +		}
> > > > > +	} else if (locked) {
> > > > > +		err = f2fs_get_block(&dn, index);
> > > > > +	} else {
> > > > > +		if (f2fs_lookup_extent_cache(inode, index, &ei)) {
> > > > > +			dn.data_blkaddr = ei.blk + index - ei.fofs;
> > > > > +		} else {
> > > > > +			bool restart = false;
> > > > > +
> > > > > +			/* hole case */
> > > > > +			err = get_dnode_of_data(&dn, index, LOOKUP_NODE);
> > > >
> > > > Better to handle error case like -ENOMEM or -EIO.
> > >
> > > I think we can just give another chance at this moment and let f2fs_get_block()
> > > handle that in the next round.
> > >
> > > Thanks,
> > >
> > > >
> > > > Thanks,
> > > >
> > > > > +			if (err || (!err && dn.data_blkaddr == NULL_ADDR))
> > > > > +				restart = true;
> > > > > +			if (restart) {
> > > > > +				f2fs_put_dnode(&dn);
> > > > > +				f2fs_lock_op(sbi);
> > > > > +				locked = true;
> > > > > +				goto restart;
> > > > > +			}
> > > > >  		}
> > > > >  	}
> > > > > -	err = f2fs_get_block(&dn, index);
> > > > > -done:
> > > > > +
> > > > >  	/* convert_inline_page can make node_changed */
> > > > >  	*blk_addr = dn.data_blkaddr;
> > > > >  	*node_changed = dn.node_changed;
> > > > > -err_out:
> > > > > +out:
> > > > >  	f2fs_put_dnode(&dn);
> > > > >  unlock_out:
> > > > > -	f2fs_unlock_op(sbi);
> > > > > +	if (locked)
> > > > > +		f2fs_unlock_op(sbi);
> > > > >  	return err;
> > > > >  }
> > > > >
> > > > > @@ -1488,7 +1513,7 @@ repeat:
> > > > >
> > > > >  	*pagep = page;
> > > > >
> > > > > -	err = prepare_write_begin(sbi, page, pos + len,
> > > > > +	err = prepare_write_begin(sbi, page, pos,  len,
> > > > >  					&blkaddr, &need_balance);
> > > > >  	if (err)
> > > > >  		goto fail;
> > > > > --
> > > > > 2.5.4 (Apple Git-61)
> > > > >
> > > > >
> > > > > ------------------------------------------------------------------------------
> > > > > _______________________________________________
> > > > > Linux-f2fs-devel mailing list
> > > > > Linux-f2fs-devel@lists.sourceforge.net
> > > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> > >
> > > ------------------------------------------------------------------------------
> > > _______________________________________________
> > > Linux-f2fs-devel mailing list
> > > Linux-f2fs-devel@lists.sourceforge.net
> > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web