Hi, Christoph Hellwig
resend: + cc lkml, linux-fsdevel
Since there is no response for my last mail, I worry that some problem in
the mail system, please allow me to resend it.
I found regression in v4.0-rc1 caused by this patch:
Author: Christoph Hellwig <[email protected]>
Date: Wed Jan 14 10:42:36 2015 +0100
fs: export inode_to_bdi and use it in favor of mapping->backing_dev_info
Test process is following:
2015-02-25 15:50:22: Start
2015-02-25 15:50:22: Linux version:Linux btrfs 4.0.0-rc1_HEAD_c517d838eb7d07bbe9507871fab3931deccff539_ #1 SMP Wed Feb 25 10:59:10 CST 2015 x86_64 x86_64 x86_64 GNU/Linux
2015-02-25 15:50:25: mkfs.btrfs -f /dev/sdb1
2015-02-25 15:50:27: mount /dev/sdb1 /data/ltf/tester
2015-02-25 15:50:28: sysbench --test=fileio --num-threads=1 --file-num=1 --file-block-size=32768 --file-total-size=4G --file-test-mode=seqwr --file-io-mode=sync --file-extra-flags= --file-fsync-freq=0 --file-fsync-end=off --max-requests=131072
2015-02-25 15:51:40: done sysbench
Result is following:
v3.19-rc1: testcnt=40 average=135.677 range=[132.460,139.130] stdev=1.610 cv=1.19%
v4.0-rc1: testcnt=40 average=130.970 range=[127.980,132.050] stdev=1.012 cv=0.77%
Then I bisect above case between v3.19-rc1 and v4.0-rc1, and found this patch caused the regresstion.
Maybe it is because kernel need more time to call node_to_bdi(), compared with "using inode->i_mapping->backing_dev_info directly" in old code.
Is there some way to speed up it(inline, or some access some variant in struct directly, ...)?
--Relative data--
Performance data before and after this patch:
v3.19-rc5_00005_495a27 : io_speed: valcnt=10 avg=137.409 range=[134.820,139.000] diff=3.10% stdev=1.574 cv=1.15%
v3.19-rc5_00006_26ff13 : io_speed: valcnt=10 avg=136.534 range=[132.390,139.500] diff=5.37% stdev=2.659 cv=1.95%
v3.19-rc5_00007_de1414 : io_speed: valcnt=10 avg=130.358 range=[129.070,132.150] diff=2.39% stdev=1.120 cv=0.86% <- *this patch*
v3.19-rc5_00008_b83ae6 : io_speed: valcnt=10 avg=129.549 range=[129.200,129.910] diff=0.55% stdev=0.241 cv=0.19%
v3.19-rc5_00011_c4db59 : io_speed: valcnt=10 avg=130.033 range=[129.050,131.620] diff=1.99% stdev=0.854 cv=0.66%
sysbench's detail log in testing:
sysbench 0.4.12: multi-threaded system evaluation benchmark
1 files, 4194304Kb each, 4096Mb total
Creating files for the test...
sysbench 0.4.12: multi-threaded system evaluation benchmark
Running the test with following options:
Number of threads: 1
Extra file open flags: 0
1 files, 4Gb each
4Gb total file size
Block size 32Kb
Using synchronous I/O mode
Doing sequential write (creation) test
Threads started!
Done.
Operations performed: 0 Read, 131072 Write, 0 Other = 131072 Total Read 0b Written 4Gb Total transferred 4Gb (132.15Mb/sec)
4228.75 Requests/sec executed
Test execution summary:
total time: 30.9955s
total number of events: 131072
total time taken by event execution: 30.8731
per-request statistics:
min: 0.01ms
avg: 0.24ms
max: 30.80ms
approx. 95 percentile: 0.03ms
Threads fairness:
events (avg/stddev): 131072.0000/0.00
execution time (avg/stddev): 30.8731/0.00
sysbench 0.4.12: multi-threaded system evaluation benchmark
My test env:
2G mem, 2-core machin, test is running on 1T sata disk.
[root@btrfs test_nosync_32768__sync_1_seqwr_4G_btrfs_1]# cat /proc/meminfo
MemTotal: 2015812 kB
MemFree: 627416 kB
MemAvailable: 1755488 kB
Buffers: 345876 kB
Cached: 772788 kB
SwapCached: 0 kB
Active: 848864 kB
Inactive: 320044 kB
Active(anon): 54128 kB
Inactive(anon): 5080 kB
Active(file): 794736 kB
Inactive(file): 314964 kB
Unevictable: 0 kB
Mlocked: 0 kB
SwapTotal: 0 kB
SwapFree: 0 kB
Dirty: 0 kB
Writeback: 0 kB
AnonPages: 50140 kB
Mapped: 41636 kB
Shmem: 8984 kB
Slab: 200312 kB
SReclaimable: 187308 kB
SUnreclaim: 13004 kB
KernelStack: 1728 kB
PageTables: 4056 kB
NFS_Unstable: 0 kB
Bounce: 0 kB
WritebackTmp: 0 kB
CommitLimit: 1007904 kB
Committed_AS: 205956 kB
VmallocTotal: 34359738367 kB
VmallocUsed: 539968 kB
VmallocChunk: 34359195223 kB
HardwareCorrupted: 0 kB
AnonHugePages: 6144 kB
HugePages_Total: 0
HugePages_Free: 0
HugePages_Rsvd: 0
HugePages_Surp: 0
Hugepagesize: 2048 kB
DirectMap4k: 61056 kB
DirectMap2M: 2000896 kB
[root@btrfs test_nosync_32768__sync_1_seqwr_4G_btrfs_1]# cat /proc/cpuinfo
processor : 0
vendor_id : GenuineIntel
cpu family : 6
model : 23
model name : Intel(R) Core(TM)2 Duo CPU E7500 @ 2.93GHz
stepping : 10
microcode : 0xa0b
cpu MHz : 1603.000
cache size : 3072 KB
physical id : 0
siblings : 2
core id : 0
cpu cores : 2
apicid : 0
initial apicid : 0
fpu : yes
fpu_exception : yes
cpuid level : 13
wp : yes
flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx lm constant_tsc arch_perfmon pebs bts rep_good nopl aperfmperf pni dtes64 monitor ds_cpl vmx est tm2 ssse3 cx16 xtpr pdcm sse4_1 xsave lahf_lm dtherm tpr_shadow vnmi flexpriority
bugs :
bogomips : 5851.89
clflush size : 64
cache_alignment : 64
address sizes : 36 bits physical, 48 bits virtual
power management:
processor : 1
vendor_id : GenuineIntel
cpu family : 6
model : 23
model name : Intel(R) Core(TM)2 Duo CPU E7500 @ 2.93GHz
stepping : 10
microcode : 0xa0b
cpu MHz : 1603.000
cache size : 3072 KB
physical id : 0
siblings : 2
core id : 1
cpu cores : 2
apicid : 1
initial apicid : 1
fpu : yes
fpu_exception : yes
cpuid level : 13
wp : yes
flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx lm constant_tsc arch_perfmon pebs bts rep_good nopl aperfmperf pni dtes64 monitor ds_cpl vmx est tm2 ssse3 cx16 xtpr pdcm sse4_1 xsave lahf_lm dtherm tpr_shadow vnmi flexpriority
bugs :
bogomips : 5851.89
clflush size : 64
cache_alignment : 64
address sizes : 36 bits physical, 48 bits virtual
power management:
Thanks
Zhaolei
On 04/10/2015 02:25 PM, Zhao Lei wrote:
> Hi, Christoph Hellwig
>
> resend: + cc lkml, linux-fsdevel
>
> Since there is no response for my last mail, I worry that some problem in
> the mail system, please allow me to resend it.
>
> I found regression in v4.0-rc1 caused by this patch:
> Author: Christoph Hellwig <[email protected]>
> Date: Wed Jan 14 10:42:36 2015 +0100
> fs: export inode_to_bdi and use it in favor of mapping->backing_dev_info
>
<>
> Result is following:
> v3.19-rc1: testcnt=40 average=135.677 range=[132.460,139.130] stdev=1.610 cv=1.19%
> v4.0-rc1: testcnt=40 average=130.970 range=[127.980,132.050] stdev=1.012 cv=0.77%
>
> Then I bisect above case between v3.19-rc1 and v4.0-rc1, and found
> this patch caused the regresstion.
>
> Maybe it is because kernel need more time to call node_to_bdi(),
> compared with "using inode->i_mapping->backing_dev_info directly" in
> old code.
>
> Is there some way to speed up it(inline, or some access some variant
> in struct directly, ...)?
>
Christoph hi
Both node_to_bdi() and sb_is_blkdev_sb()
(and I_BDEV() && blk_get_backing_dev_info())
Are an exported function calls.
Can we not make blockdev_superblock->s_bdi == NULL,
and then optimize-out the call to sb_is_blkdev_sb() to only
that case. Something like:
---
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 32a8bbd..e0375e1 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -78,7 +78,7 @@ int writeback_in_progress(struct backing_dev_info *bdi)
}
EXPORT_SYMBOL(writeback_in_progress);
-struct backing_dev_info *inode_to_bdi(struct inode *inode)
+struct backing_dev_info *__inode_to_bdi(struct inode *inode)
{
struct super_block *sb;
@@ -92,7 +92,7 @@ struct backing_dev_info *inode_to_bdi(struct inode *inode)
#endif
return sb->s_bdi;
}
-EXPORT_SYMBOL_GPL(inode_to_bdi);
+EXPORT_SYMBOL_GPL(__inode_to_bdi);
static inline struct inode *wb_inode(struct list_head *head)
{
diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
index aff923a..7d172f5 100644
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -107,7 +107,16 @@ struct backing_dev_info {
#endif
};
-struct backing_dev_info *inode_to_bdi(struct inode *inode);
+struct backing_dev_info *__inode_to_bdi(struct inode *inode);
+
+static inline
+struct backing_dev_info *inode_to_bdi(struct inode *inode)
+{
+ if (!inode || !inode->i_sb)
+ return __inode_to_bdi(inode);
+
+ return inode->i_sb->s_bdi;
+}
int __must_check bdi_init(struct backing_dev_info *bdi);
void bdi_destroy(struct backing_dev_info *bdi);
On 04/12/2015 02:33 PM, Boaz Harrosh wrote:
> On 04/10/2015 02:25 PM, Zhao Lei wrote:
>> Hi, Christoph Hellwig
>>
<>
>>
>> Is there some way to speed up it(inline, or some access some variant
>> in struct directly, ...)?
>>
>
> Christoph hi
>
> Both node_to_bdi() and sb_is_blkdev_sb()
> (and I_BDEV() && blk_get_backing_dev_info())
> Are an exported function calls.
>
> Can we not make blockdev_superblock->s_bdi == NULL,
> and then optimize-out the call to sb_is_blkdev_sb() to only
> that case. Something like:
>
> ---
>
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index 32a8bbd..e0375e1 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -78,7 +78,7 @@ int writeback_in_progress(struct backing_dev_info *bdi)
> }
> EXPORT_SYMBOL(writeback_in_progress);
>
> -struct backing_dev_info *inode_to_bdi(struct inode *inode)
> +struct backing_dev_info *__inode_to_bdi(struct inode *inode)
> {
> struct super_block *sb;
>
> @@ -92,7 +92,7 @@ struct backing_dev_info *inode_to_bdi(struct inode *inode)
> #endif
> return sb->s_bdi;
> }
> -EXPORT_SYMBOL_GPL(inode_to_bdi);
> +EXPORT_SYMBOL_GPL(__inode_to_bdi);
>
> static inline struct inode *wb_inode(struct list_head *head)
> {
> diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
> index aff923a..7d172f5 100644
> --- a/include/linux/backing-dev.h
> +++ b/include/linux/backing-dev.h
> @@ -107,7 +107,16 @@ struct backing_dev_info {
> #endif
> };
>
> -struct backing_dev_info *inode_to_bdi(struct inode *inode);
> +struct backing_dev_info *__inode_to_bdi(struct inode *inode);
> +
> +static inline
> +struct backing_dev_info *inode_to_bdi(struct inode *inode)
> +{
> + if (!inode || !inode->i_sb)
> + return __inode_to_bdi(inode);
> +
> + return inode->i_sb->s_bdi;
> +}
>
This patch actually boots. Lei could you please test to see
if it fixes your slowness?
Thanks
Boaz
> int __must_check bdi_init(struct backing_dev_info *bdi);
> void bdi_destroy(struct backing_dev_info *bdi);
>
Hi, Boaz
> -----Original Message-----
> From: Boaz Harrosh [mailto:[email protected]]
> Sent: Sunday, April 12, 2015 10:39 PM
> To: Boaz Harrosh; Zhao Lei; 'Christoph Hellwig'
> Cc: [email protected]; 'Jan Kara'; 'Jens Axboe'; 'LKML'
> Subject: Re: Regression caused by using node_to_bdi()
>
> On 04/12/2015 02:33 PM, Boaz Harrosh wrote:
> > On 04/10/2015 02:25 PM, Zhao Lei wrote:
> >> Hi, Christoph Hellwig
> >>
> <>
> >>
> >> Is there some way to speed up it(inline, or some access some variant
> >> in struct directly, ...)?
> >>
> >
> > Christoph hi
> >
> > Both node_to_bdi() and sb_is_blkdev_sb() (and I_BDEV() &&
> > blk_get_backing_dev_info()) Are an exported function calls.
> >
> > Can we not make blockdev_superblock->s_bdi == NULL, and then
> > optimize-out the call to sb_is_blkdev_sb() to only that case.
> > Something like:
> >
> > ---
> >
> > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c index
> > 32a8bbd..e0375e1 100644
> > --- a/fs/fs-writeback.c
> > +++ b/fs/fs-writeback.c
> > @@ -78,7 +78,7 @@ int writeback_in_progress(struct backing_dev_info
> > *bdi) } EXPORT_SYMBOL(writeback_in_progress);
> >
> > -struct backing_dev_info *inode_to_bdi(struct inode *inode)
> > +struct backing_dev_info *__inode_to_bdi(struct inode *inode)
> > {
> > struct super_block *sb;
> >
> > @@ -92,7 +92,7 @@ struct backing_dev_info *inode_to_bdi(struct inode
> > *inode) #endif
> > return sb->s_bdi;
> > }
> > -EXPORT_SYMBOL_GPL(inode_to_bdi);
> > +EXPORT_SYMBOL_GPL(__inode_to_bdi);
> >
> > static inline struct inode *wb_inode(struct list_head *head) { diff
> > --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
> > index aff923a..7d172f5 100644
> > --- a/include/linux/backing-dev.h
> > +++ b/include/linux/backing-dev.h
> > @@ -107,7 +107,16 @@ struct backing_dev_info { #endif };
> >
> > -struct backing_dev_info *inode_to_bdi(struct inode *inode);
> > +struct backing_dev_info *__inode_to_bdi(struct inode *inode);
> > +
> > +static inline
> > +struct backing_dev_info *inode_to_bdi(struct inode *inode) {
> > + if (!inode || !inode->i_sb)
> > + return __inode_to_bdi(inode);
> > +
> > + return inode->i_sb->s_bdi;
> > +}
> >
>
> This patch actually boots. Lei could you please test to see if it fixes your
> slowness?
>
Thanks, I'll test this patch.
Thanks
Zhaolei
> Thanks
> Boaz
>
> > int __must_check bdi_init(struct backing_dev_info *bdi); void
> > bdi_destroy(struct backing_dev_info *bdi);
> >
Hi, Boaz
> -----Original Message-----
> From: Boaz Harrosh [mailto:[email protected]]
> Sent: Sunday, April 12, 2015 10:39 PM
> To: Boaz Harrosh; Zhao Lei; 'Christoph Hellwig'
> Cc: [email protected]; 'Jan Kara'; 'Jens Axboe'; 'LKML'
> Subject: Re: Regression caused by using node_to_bdi()
>
> On 04/12/2015 02:33 PM, Boaz Harrosh wrote:
> > On 04/10/2015 02:25 PM, Zhao Lei wrote:
> >> Hi, Christoph Hellwig
> >>
> <>
> >>
> >> Is there some way to speed up it(inline, or some access some variant
> >> in struct directly, ...)?
> >>
> >
> > Christoph hi
> >
> > Both node_to_bdi() and sb_is_blkdev_sb() (and I_BDEV() &&
> > blk_get_backing_dev_info()) Are an exported function calls.
> >
> > Can we not make blockdev_superblock->s_bdi == NULL, and then
> > optimize-out the call to sb_is_blkdev_sb() to only that case.
> > Something like:
> >
> > ---
> >
> > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c index
> > 32a8bbd..e0375e1 100644
> > --- a/fs/fs-writeback.c
> > +++ b/fs/fs-writeback.c
> > @@ -78,7 +78,7 @@ int writeback_in_progress(struct backing_dev_info
> > *bdi) } EXPORT_SYMBOL(writeback_in_progress);
> >
> > -struct backing_dev_info *inode_to_bdi(struct inode *inode)
> > +struct backing_dev_info *__inode_to_bdi(struct inode *inode)
> > {
> > struct super_block *sb;
> >
> > @@ -92,7 +92,7 @@ struct backing_dev_info *inode_to_bdi(struct inode
> > *inode) #endif
> > return sb->s_bdi;
> > }
> > -EXPORT_SYMBOL_GPL(inode_to_bdi);
> > +EXPORT_SYMBOL_GPL(__inode_to_bdi);
> >
> > static inline struct inode *wb_inode(struct list_head *head) { diff
> > --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
> > index aff923a..7d172f5 100644
> > --- a/include/linux/backing-dev.h
> > +++ b/include/linux/backing-dev.h
> > @@ -107,7 +107,16 @@ struct backing_dev_info { #endif };
> >
> > -struct backing_dev_info *inode_to_bdi(struct inode *inode);
> > +struct backing_dev_info *__inode_to_bdi(struct inode *inode);
> > +
> > +static inline
> > +struct backing_dev_info *inode_to_bdi(struct inode *inode) {
> > + if (!inode || !inode->i_sb)
> > + return __inode_to_bdi(inode);
> > +
> > + return inode->i_sb->s_bdi;
> > +}
> >
>
> This patch actually boots. Lei could you please test to see if it fixes your
> slowness?
>
The good news is this patch passed compile and 10-time tests.
The bad news is it have more performance down(strange)...
v3.19-rc1 : io_speed: valcnt=10 avg=214.688 range=[211.460,216.190] diff= 2.24% stdev=1.417 cv=0.66%
v4.0-rc1 : io_speed: valcnt=10 avg=204.917 range=[203.370,205.890] diff= 1.24% stdev=0.663 cv=0.32%
v4.0-rc1_00001_82ad06 : io_speed: valcnt=10 avg=189.337 range=[186.280,192.060] diff= 3.10% stdev=2.305 cv=1.22% *<- this patch
I applied this patch on top of v4.0-rc1.
> Thanks
> Boaz
>
> > int __must_check bdi_init(struct backing_dev_info *bdi); void
> > bdi_destroy(struct backing_dev_info *bdi);
> >
Hi, Boaz
> -----Original Message-----
> From: Zhao Lei [mailto:[email protected]]
> Sent: Monday, April 13, 2015 3:00 PM
> To: 'Boaz Harrosh'; 'Christoph Hellwig'
> Cc: '[email protected]'; 'Jan Kara'; 'Jens Axboe'; 'LKML'
> Subject: RE: Regression caused by using node_to_bdi()
>
> Hi, Boaz
>
> > -----Original Message-----
> > From: Boaz Harrosh [mailto:[email protected]]
> > Sent: Sunday, April 12, 2015 10:39 PM
> > To: Boaz Harrosh; Zhao Lei; 'Christoph Hellwig'
> > Cc: [email protected]; 'Jan Kara'; 'Jens Axboe'; 'LKML'
> > Subject: Re: Regression caused by using node_to_bdi()
> >
> > On 04/12/2015 02:33 PM, Boaz Harrosh wrote:
> > > On 04/10/2015 02:25 PM, Zhao Lei wrote:
> > >> Hi, Christoph Hellwig
> > >>
> > <>
> > >>
> > >> Is there some way to speed up it(inline, or some access some
> > >> variant in struct directly, ...)?
> > >>
> > >
> > > Christoph hi
> > >
> > > Both node_to_bdi() and sb_is_blkdev_sb() (and I_BDEV() &&
> > > blk_get_backing_dev_info()) Are an exported function calls.
> > >
> > > Can we not make blockdev_superblock->s_bdi == NULL, and then
> > > optimize-out the call to sb_is_blkdev_sb() to only that case.
> > > Something like:
> > >
> > > ---
> > >
> > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c index
> > > 32a8bbd..e0375e1 100644
> > > --- a/fs/fs-writeback.c
> > > +++ b/fs/fs-writeback.c
> > > @@ -78,7 +78,7 @@ int writeback_in_progress(struct backing_dev_info
> > > *bdi) } EXPORT_SYMBOL(writeback_in_progress);
> > >
> > > -struct backing_dev_info *inode_to_bdi(struct inode *inode)
> > > +struct backing_dev_info *__inode_to_bdi(struct inode *inode)
> > > {
> > > struct super_block *sb;
> > >
> > > @@ -92,7 +92,7 @@ struct backing_dev_info *inode_to_bdi(struct inode
> > > *inode) #endif
> > > return sb->s_bdi;
> > > }
> > > -EXPORT_SYMBOL_GPL(inode_to_bdi);
> > > +EXPORT_SYMBOL_GPL(__inode_to_bdi);
> > >
> > > static inline struct inode *wb_inode(struct list_head *head) {
> > > diff --git a/include/linux/backing-dev.h
> > > b/include/linux/backing-dev.h index aff923a..7d172f5 100644
> > > --- a/include/linux/backing-dev.h
> > > +++ b/include/linux/backing-dev.h
> > > @@ -107,7 +107,16 @@ struct backing_dev_info { #endif };
> > >
> > > -struct backing_dev_info *inode_to_bdi(struct inode *inode);
> > > +struct backing_dev_info *__inode_to_bdi(struct inode *inode);
> > > +
> > > +static inline
> > > +struct backing_dev_info *inode_to_bdi(struct inode *inode) {
> > > + if (!inode || !inode->i_sb)
> > > + return __inode_to_bdi(inode);
> > > +
> > > + return inode->i_sb->s_bdi;
> > > +}
> > >
> >
> > This patch actually boots. Lei could you please test to see if it
> > fixes your slowness?
> >
> The good news is this patch passed compile and 10-time tests.
> The bad news is it have more performance down(strange)...
>
> v3.19-rc1 : io_speed: valcnt=10 avg=214.688
> range=[211.460,216.190] diff= 2.24% stdev=1.417 cv=0.66%
> v4.0-rc1 : io_speed: valcnt=10 avg=204.917
> range=[203.370,205.890] diff= 1.24% stdev=0.663 cv=0.32%
> v4.0-rc1_00001_82ad06 : io_speed: valcnt=10 avg=189.337
> range=[186.280,192.060] diff= 3.10% stdev=2.305 cv=1.22% *<- this patch
>
> I applied this patch on top of v4.0-rc1.
>
A new bad news:
This patch make filesystem unstable.
My env entered to following command line in booting after
apply this patch to v4.0-rc1:
Welcome to emergency mode! After logging in, type "journalctl -xb: to view
System logs, ...
Give root password for maintenance
(or press Control-D to continue)
I confirmed this error message for more than 3 times.
(and confirmed no-problem without this patch)
In previous performance test(which get result in my last mail), I hadn't
pay attention to that message, and just type Ctrl-D and begin test.
Thanks
Zhaolei
> > Thanks
> > Boaz
> >
> > > int __must_check bdi_init(struct backing_dev_info *bdi); void
> > > bdi_destroy(struct backing_dev_info *bdi);
> > >
On Sun 12-04-15 14:33:12, Boaz Harrosh wrote:
> On 04/10/2015 02:25 PM, Zhao Lei wrote:
> > Hi, Christoph Hellwig
> >
> > resend: + cc lkml, linux-fsdevel
> >
> > Since there is no response for my last mail, I worry that some problem in
> > the mail system, please allow me to resend it.
> >
> > I found regression in v4.0-rc1 caused by this patch:
> > Author: Christoph Hellwig <[email protected]>
> > Date: Wed Jan 14 10:42:36 2015 +0100
> > fs: export inode_to_bdi and use it in favor of mapping->backing_dev_info
> >
> <>
> > Result is following:
> > v3.19-rc1: testcnt=40 average=135.677 range=[132.460,139.130] stdev=1.610 cv=1.19%
> > v4.0-rc1: testcnt=40 average=130.970 range=[127.980,132.050] stdev=1.012 cv=0.77%
> >
> > Then I bisect above case between v3.19-rc1 and v4.0-rc1, and found
> > this patch caused the regresstion.
> >
> > Maybe it is because kernel need more time to call node_to_bdi(),
> > compared with "using inode->i_mapping->backing_dev_info directly" in
> > old code.
> >
> > Is there some way to speed up it(inline, or some access some variant
> > in struct directly, ...)?
> >
>
> Christoph hi
>
> Both node_to_bdi() and sb_is_blkdev_sb()
> (and I_BDEV() && blk_get_backing_dev_info())
> Are an exported function calls.
>
> Can we not make blockdev_superblock->s_bdi == NULL,
> and then optimize-out the call to sb_is_blkdev_sb() to only
> that case. Something like:
>
> ---
>
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index 32a8bbd..e0375e1 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -78,7 +78,7 @@ int writeback_in_progress(struct backing_dev_info *bdi)
> }
> EXPORT_SYMBOL(writeback_in_progress);
>
> -struct backing_dev_info *inode_to_bdi(struct inode *inode)
> +struct backing_dev_info *__inode_to_bdi(struct inode *inode)
> {
> struct super_block *sb;
>
> @@ -92,7 +92,7 @@ struct backing_dev_info *inode_to_bdi(struct inode *inode)
> #endif
> return sb->s_bdi;
> }
> -EXPORT_SYMBOL_GPL(inode_to_bdi);
> +EXPORT_SYMBOL_GPL(__inode_to_bdi);
>
> static inline struct inode *wb_inode(struct list_head *head)
> {
> diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
> index aff923a..7d172f5 100644
> --- a/include/linux/backing-dev.h
> +++ b/include/linux/backing-dev.h
> @@ -107,7 +107,16 @@ struct backing_dev_info {
> #endif
> };
>
> -struct backing_dev_info *inode_to_bdi(struct inode *inode);
> +struct backing_dev_info *__inode_to_bdi(struct inode *inode);
> +
> +static inline
> +struct backing_dev_info *inode_to_bdi(struct inode *inode)
> +{
> + if (!inode || !inode->i_sb)
> + return __inode_to_bdi(inode);
> +
> + return inode->i_sb->s_bdi;
> +}
This is wrong for block-device inodes, isn't it?
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On 04/13/2015 01:22 PM, Zhao Lei wrote:
<>
> A new bad news:
> This patch make filesystem unstable.
>
Rrrr yes sorry Lei. Why this boots my systems is not clear this is not
what I intended to write.
Here is what I meant to write (replacing the old one):
----
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 32a8bbd..e0375e1 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -78,7 +78,7 @@ int writeback_in_progress(struct backing_dev_info *bdi)
}
EXPORT_SYMBOL(writeback_in_progress);
-struct backing_dev_info *inode_to_bdi(struct inode *inode)
+struct backing_dev_info *__inode_to_bdi(struct inode *inode)
{
struct super_block *sb;
@@ -92,7 +92,7 @@ struct backing_dev_info *inode_to_bdi(struct inode *inode)
#endif
return sb->s_bdi;
}
-EXPORT_SYMBOL_GPL(inode_to_bdi);
+EXPORT_SYMBOL_GPL(__inode_to_bdi);
static inline struct inode *wb_inode(struct list_head *head)
{
diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
index aff923a..53d97cd 100644
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -107,7 +107,16 @@ struct backing_dev_info {
#endif
};
-struct backing_dev_info *inode_to_bdi(struct inode *inode);
+struct backing_dev_info *__inode_to_bdi(struct inode *inode);
+
+static inline
+struct backing_dev_info *inode_to_bdi(struct inode *inode)
+{
+ if (!inode || !inode->i_sb || !inode->i_sb->s_bdi)
+ return __inode_to_bdi(inode);
+
+ return inode->i_sb->s_bdi;
+}
int __must_check bdi_init(struct backing_dev_info *bdi);
void bdi_destroy(struct backing_dev_info *bdi);
On 04/13/2015 03:21 PM, Jan Kara wrote:
<>
>> -struct backing_dev_info *inode_to_bdi(struct inode *inode);
>> +struct backing_dev_info *__inode_to_bdi(struct inode *inode);
>> +
>> +static inline
>> +struct backing_dev_info *inode_to_bdi(struct inode *inode)
>> +{
>> + if (!inode || !inode->i_sb)
>> + return __inode_to_bdi(inode);
>> +
>> + return inode->i_sb->s_bdi;
>> +}
> This is wrong for block-device inodes, isn't it?
Rrr yes my bad I meant
+struct backing_dev_info *inode_to_bdi(struct inode *inode)
+{
+ if (!inode || !inode->i_sb || !inode->i_sb->s_bdi)
+ return __inode_to_bdi(inode);
+
+ return inode->i_sb->s_bdi;
+}
I was hopping that blockdev_superblock->s_bdi == NULL
because what sb_is_blkdev_sb() is doing is checking
for blockdev_superblock.
>From code audit I do not see where it might be set but
I might have missed it.
Thanks Jan
Boaz
>
> Honza
>
Tejun posted a series last week that moves node_to_bdi back inline
to -fsdevel.
Can you apply patches 1-18 of the series
"[PATCHSET 1/3 v3 block/for-4.1/core] writeback: cgroup writeback support"
and see if that helps your benchmark?
Hi, Boaz
> -----Original Message-----
> From: Boaz Harrosh [mailto:[email protected]]
> Sent: Monday, April 13, 2015 8:31 PM
> To: Zhao Lei; 'Boaz Harrosh'; 'Christoph Hellwig'
> Cc: [email protected]; 'Jan Kara'; 'Jens Axboe'; 'LKML'
> Subject: Re: Regression caused by using node_to_bdi()
>
> On 04/13/2015 01:22 PM, Zhao Lei wrote:
> <>
> > A new bad news:
> > This patch make filesystem unstable.
> >
>
> Rrrr yes sorry Lei. Why this boots my systems is not clear this is not what I
> intended to write.
>
> Here is what I meant to write (replacing the old one):
Thanks for v2 patch.
But is still halt the test machine with same error message in boot.
I applied the patch on v4.0-rc1, not just after the patch of
introduce node_to_bdi() in this mail.
Is way of inode->i_sb->s_bdi changed after we move to node_to_bdi()?
Thanks
Zhaolei
> ----
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c index 32a8bbd..e0375e1
> 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -78,7 +78,7 @@ int writeback_in_progress(struct backing_dev_info
> *bdi) } EXPORT_SYMBOL(writeback_in_progress);
>
> -struct backing_dev_info *inode_to_bdi(struct inode *inode)
> +struct backing_dev_info *__inode_to_bdi(struct inode *inode)
> {
> struct super_block *sb;
>
> @@ -92,7 +92,7 @@ struct backing_dev_info *inode_to_bdi(struct inode
> *inode) #endif
> return sb->s_bdi;
> }
> -EXPORT_SYMBOL_GPL(inode_to_bdi);
> +EXPORT_SYMBOL_GPL(__inode_to_bdi);
>
> static inline struct inode *wb_inode(struct list_head *head) { diff --git
> a/include/linux/backing-dev.h b/include/linux/backing-dev.h index
> aff923a..53d97cd 100644
> --- a/include/linux/backing-dev.h
> +++ b/include/linux/backing-dev.h
> @@ -107,7 +107,16 @@ struct backing_dev_info { #endif };
>
> -struct backing_dev_info *inode_to_bdi(struct inode *inode);
> +struct backing_dev_info *__inode_to_bdi(struct inode *inode);
> +
> +static inline
> +struct backing_dev_info *inode_to_bdi(struct inode *inode) {
> + if (!inode || !inode->i_sb || !inode->i_sb->s_bdi)
> + return __inode_to_bdi(inode);
> +
> + return inode->i_sb->s_bdi;
> +}
>
> int __must_check bdi_init(struct backing_dev_info *bdi); void
> bdi_destroy(struct backing_dev_info *bdi);
>
Hi, Christoph
> -----Original Message-----
> From: 'Christoph Hellwig' [mailto:[email protected]]
> Sent: Tuesday, April 14, 2015 1:33 AM
> To: Zhao Lei
> Cc: 'Christoph Hellwig'; [email protected]; 'Jan Kara'; 'Jens Axboe';
> 'LKML'
> Subject: Re: Regression caused by using node_to_bdi()
>
> Tejun posted a series last week that moves node_to_bdi back inline to -fsdevel.
>
> Can you apply patches 1-18 of the series
>
> "[PATCHSET 1/3 v3 block/for-4.1/core] writeback: cgroup writeback support"
>
> and see if that helps your benchmark?
I done performance test for this patchset(1~18).
But the result is not changed, performace is not increased:
v3.19-rc1 : io_speed: valcnt=10 avg=214.688 range=[211.460,216.190] diff= 2.24% stdev=1.417 cv=0.66%
v3.19 : io_speed: valcnt=10 avg=212.529 range=[206.160,216.980] diff= 5.25% stdev=3.588 cv=1.69%
# This is 3 patch before
v3.19_04754_1b30e6 : io_speed: valcnt=10 avg=214.072 range=[210.690,217.440] diff= 3.20% stdev=2.222 cv=1.04%
v3.19_04765_495a27 : io_speed: valcnt=10 avg=210.163 range=[205.130,215.630] diff= 5.12% stdev=3.140 cv=1.49%
v3.19_04766_26ff13 : io_speed: valcnt=10 avg=210.045 range=[207.590,212.550] diff= 2.39% stdev=1.860 cv=0.89%
# This is patch of using node_to_bdi()
v3.19_04767_de1414 : io_speed: valcnt=10 avg=205.216 range=[202.870,206.620] diff= 1.85% stdev=0.977 cv=0.48%
# This is 3 patch after
v3.19_04768_b83ae6 : io_speed: valcnt=10 avg=205.265 range=[203.720,206.170] diff= 1.20% stdev=0.721 cv=0.35%
v3.19_04769_e4d275 : io_speed: valcnt=10 avg=205.787 range=[205.250,206.150] diff= 0.44% stdev=0.306 cv=0.15%
v3.19_04770_7b14a2 : io_speed: valcnt=10 avg=203.558 range=[203.110,204.020] diff= 0.45% stdev=0.308 cv=0.15%
v4.0-rc1 : io_speed: valcnt=10 avg=204.917 range=[203.370,205.890] diff= 1.24% stdev=0.663 cv=0.32%
# This is 1~18 of above patchset on top of v4.0-rc1
v4.0-rc1_00018_579465 : io_speed: valcnt=10 avg=205.348 range=[204.430,206.120] diff= 0.83% stdev=0.572 cv=0.28%
v4.0 : io_speed: valcnt=10 avg=205.442 range=[204.680,207.290] diff= 1.28% stdev=0.831 cv=0.40%
(Above performance data is changed with my first mail,
because the old harddisk was broken, then I replaced to a
new one and run all performance test again)
Thanks
Zhaolei