| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20130428115313.GA3405@gmail.com>
Date: Sun, 28 Apr 2013 19:53:13 +0800
From: Zheng Liu <gnehzuil.liu@...il.com>
To: "Yan, Zheng" <zheng.z.yan@...el.com>
Cc: linux-ext4@...r.kernel.org, wenqing.lz@...bao.com, tytso@....edu,
lkp@...ux.intel.com
Subject: Re: [PATCH] ext4: fix fio regression
On Sun, Apr 28, 2013 at 07:21:51PM +0800, Yan, Zheng wrote:
[snip]
> > Subject: [PATCH] ext4: fix fio regression
> >
> > From: "Yan, Zheng" <zheng.z.yan@...el.com>
> >
> > We (Linux Kernel Performance project) found a regression introduced
> > by commit:
> >
> > f7fec032aa ext4: track all extent status in extent status tree
> >
> > The commit causes about 20% performance decrease in fio random write
> > test. Profiler shows that rb_next() uses a lot of CPU time. The call
> > stack is:
> >
> > rb_next
> > ext4_es_find_delayed_extent
> > ext4_map_blocks
> > _ext4_get_block
> > ext4_get_block_write
> > __blockdev_direct_IO
> > ext4_direct_IO
> > generic_file_direct_write
> > __generic_file_aio_write
> > ext4_file_write
> > aio_rw_vect_retry
> > aio_run_iocb
> > do_io_submit
> > sys_io_submit
> > system_call_fastpath
> > io_submit
> > td_io_getevents
> > io_u_queued_complete
> > thread_main
> > main
> > __libc_start_main
> >
> > The cause is that ext4_es_find_delayed_extent() doesn't have an
> > upper bound, it keeps searching until a delayed extent is found.
> > When there are a lots of non-delayed entries in the extent state
> > tree, ext4_es_find_delayed_extent() may uses a lot of CPU time.
> >
> > Reported-by: LKP project <lkp@...ux.intel.com>
> > Signed-off-by: Yan, Zheng <zheng.z.yan@...el.com>
> > Signed-off-by: Zheng Liu <wenqing.lz@...bao.com>
> > ---
> > changelog:
> > * rename ext4_es_find_delayed_extent with ext4_es_find_delayed_extent_range
> > * add a BUG_ON in ext4_es_find_delayed_extent_range
> > * don't find a delayed extent in ext4_find_delayed_extent when es_pblk != 0
> > * search a delayed extent in a given range
> >
> > fs/ext4/extents.c | 10 ++++++----
> > fs/ext4/extents_status.c | 17 ++++++++++++-----
> > fs/ext4/extents_status.h | 3 ++-
> > fs/ext4/file.c | 4 ++--
> > include/trace/events/ext4.h | 4 ++--
> > 5 files changed, 24 insertions(+), 14 deletions(-)
> >
> > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> > index 107936d..1da3c0c 100644
> > --- a/fs/ext4/extents.c
> > +++ b/fs/ext4/extents.c
> > @@ -3642,7 +3642,7 @@ int ext4_find_delalloc_range(struct inode *inode,
> > {
> > struct extent_status es;
> >
> > - ext4_es_find_delayed_extent(inode, lblk_start, &es);
> > + ext4_es_find_delayed_extent_range(inode, lblk_start, lblk_end, &es);
> > if (es.es_len == 0)
> > return 0; /* there is no delay extent in this tree */
> > else if (es.es_lblk <= lblk_start &&
> > @@ -4608,9 +4608,11 @@ static int ext4_find_delayed_extent(struct inode *inode,
> > struct extent_status es;
> > ext4_lblk_t block, next_del;
> >
> > - ext4_es_find_delayed_extent(inode, newes->es_lblk, &es);
> > -
> > if (newes->es_pblk == 0) {
> > + ext4_es_find_delayed_extent_range(inode, newes->es_lblk,
> > + newes->es_lblk + newes->es_len,
> newes->es_lblk + newes->es_len - 1 ?
>
> Other than this, the improved version looks good.
Yes, you are right. Thanks for pointing it out.
Regards,
- Zheng
> > + &es);
> > +
> > /*
> > * No extent in extent-tree contains block @newes->es_pblk,
> > * then the block may stay in 1)a hole or 2)delayed-extent.
> > @@ -4630,7 +4632,7 @@ static int ext4_find_delayed_extent(struct inode *inode,
> > }
> >
> > block = newes->es_lblk + newes->es_len;
> > - ext4_es_find_delayed_extent(inode, block, &es);
> > + ext4_es_find_delayed_extent_range(inode, block, EXT_MAX_BLOCKS, &es);
> > if (es.es_len == 0)
> > next_del = EXT_MAX_BLOCKS;
> > else
> > diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> > index fe3337a..e6941e6 100644
> > --- a/fs/ext4/extents_status.c
> > +++ b/fs/ext4/extents_status.c
> > @@ -232,14 +232,16 @@ static struct extent_status *__es_tree_search(struct rb_root *root,
> > }
> >
> > /*
> > - * ext4_es_find_delayed_extent: find the 1st delayed extent covering @es->lblk
> > - * if it exists, otherwise, the next extent after @es->lblk.
> > + * ext4_es_find_delayed_extent_range: find the 1st delayed extent covering
> > + * @es->lblk if it exists, otherwise, the next extent after @es->lblk.
> > *
> > * @inode: the inode which owns delayed extents
> > * @lblk: the offset where we start to search
> > + * @end: the offset where we stop to search
> > * @es: delayed extent that we found
> > */
> > -void ext4_es_find_delayed_extent(struct inode *inode, ext4_lblk_t lblk,
> > +void ext4_es_find_delayed_extent_range(struct inode *inode,
> > + ext4_lblk_t lblk, ext4_lblk_t end,
> > struct extent_status *es)
> > {
> > struct ext4_es_tree *tree = NULL;
> > @@ -247,7 +249,8 @@ void ext4_es_find_delayed_extent(struct inode *inode, ext4_lblk_t lblk,
> > struct rb_node *node;
> >
> > BUG_ON(es == NULL);
> > - trace_ext4_es_find_delayed_extent_enter(inode, lblk);
> > + BUG_ON(end < lblk);
> > + trace_ext4_es_find_delayed_extent_range_enter(inode, lblk);
> >
> > read_lock(&EXT4_I(inode)->i_es_lock);
> > tree = &EXT4_I(inode)->i_es_tree;
> > @@ -270,6 +273,10 @@ out:
> > if (es1 && !ext4_es_is_delayed(es1)) {
> > while ((node = rb_next(&es1->rb_node)) != NULL) {
> > es1 = rb_entry(node, struct extent_status, rb_node);
> > + if (es1->es_lblk > end) {
> > + es1 = NULL;
> > + break;
> > + }
> > if (ext4_es_is_delayed(es1))
> > break;
> > }
> > @@ -285,7 +292,7 @@ out:
> > read_unlock(&EXT4_I(inode)->i_es_lock);
> >
> > ext4_es_lru_add(inode);
> > - trace_ext4_es_find_delayed_extent_exit(inode, es);
> > + trace_ext4_es_find_delayed_extent_range_exit(inode, es);
> > }
> >
> > static struct extent_status *
> > diff --git a/fs/ext4/extents_status.h b/fs/ext4/extents_status.h
> > index d8e2d4d..f740eb03 100644
> > --- a/fs/ext4/extents_status.h
> > +++ b/fs/ext4/extents_status.h
> > @@ -62,7 +62,8 @@ extern int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
> > unsigned long long status);
> > extern int ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
> > ext4_lblk_t len);
> > -extern void ext4_es_find_delayed_extent(struct inode *inode, ext4_lblk_t lblk,
> > +extern void ext4_es_find_delayed_extent_range(struct inode *inode,
> > + ext4_lblk_t lblk, ext4_lblk_t end,
> > struct extent_status *es);
> > extern int ext4_es_lookup_extent(struct inode *inode, ext4_lblk_t lblk,
> > struct extent_status *es);
> > diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> > index 64848b5..1bc4523 100644
> > --- a/fs/ext4/file.c
> > +++ b/fs/ext4/file.c
> > @@ -464,7 +464,7 @@ static loff_t ext4_seek_data(struct file *file, loff_t offset, loff_t maxsize)
> > * If there is a delay extent at this offset,
> > * it will be as a data.
> > */
> > - ext4_es_find_delayed_extent(inode, last, &es);
> > + ext4_es_find_delayed_extent_range(inode, last, last, &es);
> > if (es.es_len != 0 && in_range(last, es.es_lblk, es.es_len)) {
> > if (last != start)
> > dataoff = last << blkbits;
> > @@ -547,7 +547,7 @@ static loff_t ext4_seek_hole(struct file *file, loff_t offset, loff_t maxsize)
> > * If there is a delay extent at this offset,
> > * we will skip this extent.
> > */
> > - ext4_es_find_delayed_extent(inode, last, &es);
> > + ext4_es_find_delayed_extent_range(inode, last, last, &es);
> > if (es.es_len != 0 && in_range(last, es.es_lblk, es.es_len)) {
> > last = es.es_lblk + es.es_len;
> > holeoff = last << blkbits;
> > diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h
> > index d0e6864..8ee15b9 100644
> > --- a/include/trace/events/ext4.h
> > +++ b/include/trace/events/ext4.h
> > @@ -2139,7 +2139,7 @@ TRACE_EVENT(ext4_es_remove_extent,
> > __entry->lblk, __entry->len)
> > );
> >
> > -TRACE_EVENT(ext4_es_find_delayed_extent_enter,
> > +TRACE_EVENT(ext4_es_find_delayed_extent_range_enter,
> > TP_PROTO(struct inode *inode, ext4_lblk_t lblk),
> >
> > TP_ARGS(inode, lblk),
> > @@ -2161,7 +2161,7 @@ TRACE_EVENT(ext4_es_find_delayed_extent_enter,
> > (unsigned long) __entry->ino, __entry->lblk)
> > );
> >
> > -TRACE_EVENT(ext4_es_find_delayed_extent_exit,
> > +TRACE_EVENT(ext4_es_find_delayed_extent_range_exit,
> > TP_PROTO(struct inode *inode, struct extent_status *es),
> >
> > TP_ARGS(inode, es),
> >
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists