* Re: [PATCH 0/5] block: a virtual block device driver for testing
From: Shaohua Li @ 2017-08-08 21:05 UTC (permalink / raw)
To: Jens Axboe
Cc: Hannes Reinecke, linux-block, linux-raid, kernel-team,
Kyungchan Koh, Shaohua Li
In-Reply-To: <f6896ce5-1752-feba-fadb-dbc89dbf4e20@kernel.dk>
On Tue, Aug 08, 2017 at 02:31:54PM -0600, Jens Axboe wrote:
> On 08/07/2017 10:36 AM, Shaohua Li wrote:
> > On Mon, Aug 07, 2017 at 10:29:05AM +0200, Hannes Reinecke wrote:
> >> On 08/05/2017 05:51 PM, Shaohua Li wrote:
> >>> From: Shaohua Li <shli@fb.com>
> >>>
> >>> In testing software RAID, I usually found it's hard to cover specific cases.
> >>> RAID is supposed to work even disk is in semi good state, for example, some
> >>> sectors are broken. Since we can't control the behavior of hardware, it's
> >>> difficult to create test suites to do destructive tests. But we can control the
> >>> behavior of software, software based disk is an obvious choice for such tests.
> >>> While we already have several software based disks for testing (eg, null_blk,
> >>> scsi_debug), none is for destructive testing, this is the reason we create a
> >>> new test block device.
> >>>
> >>> Currently the driver can create disk with following features:
> >>> - Bandwidth control. A raid array consists of several disks. The disks could
> >>> run in different speed, for example, one disk is SSD and the other is HD.
> >>> Actually raid1 has a feature called write behind just for this. To test such
> >>> raid1 feature, we'd like the disks speed could be controlled.
> >>> - Emulate disk cache. Software must flush disk cache to guarantee data is
> >>> safely stored in media after a power failure. To verify if software works
> >>> well, we can't simply use physical disk, because even software doesn't flush
> >>> cache, the hardware probably will flush the cache. With a software
> >>> implementation of disk cache, we can fully control how we flush disk cache in a
> >>> power failure.
> >>> - Badblock. If only part of a disk is broken, software raid continues working.
> >>> To test if software raid works well, disks must include some broken parts or
> >>> bad blocks. Bad blocks can be easily implemented in software.
> >>>
> >>> While this is inspired by software raid testing, the driver is very flexible
> >>> for extension. We can easily add new features into the driver. The interface is
> >>> configfs, which can be configured with a shell script. There is a 'features'
> >>> attribute exposing all supported features. By checking this, we don't need to
> >>> worry about compability issues. For configuration details, please check the
> >>> first patch.
> >>>
> >> Any particular reason why you can't fold these changes into brd or null_blk?
> >> After all, without those testing features it is 'just' another ramdisk
> >> driver...
> >
> > null_blk isn't a good fit. ramdisk might be, but I try to not. We are adding
> > new interfaces, locks and so on. Adding the features into ramdisk driver will
> > mess it up. Binding it to ramdisk driver will make adding new features harder
> > too, because the test driver doesn't really care about performance while
> > ramdisk does.
>
> I'm curious why null_blk isn't a good fit? You'd just need to add RAM
> storage to it. That would just be a separate option that should be set,
> ram_backing=1 or something like that. That would make it less critical
> than using the RAM disk driver as well, since only people that want a "real"
> data backing would enable it.
>
> It's not that I'm extremely opposed to adding a(nother) test block driver,
> but we at least need some sort of reasoning behind why, which isn't just
> "not a good fit".
Ah, I thought the 'null' of null_blk means we do nothing for the disks. Of
course we can rename it, which means this point less meaningful. I think the
main reason is the interface. We will configure the disks with different
parameters and do power on/off for each disks (which is the key we can emulate
disk cache and power loss). The module paramter interface of null_blk doesn't
work for the usage. Of course, these issues can be fixed, for example, we can
make null_blk use the configfs interface. If you really prefer a single driver
for all test purpose, I can move the test_blk functionalities to null_blk.
Thanks,
Shaohua
^ permalink raw reply
* Re: [PATCH 0/5] block: a virtual block device driver for testing
From: Jens Axboe @ 2017-08-08 21:13 UTC (permalink / raw)
To: Shaohua Li, Jens Axboe
Cc: Hannes Reinecke, linux-block, linux-raid, kernel-team,
Kyungchan Koh, Shaohua Li
In-Reply-To: <20170808210507.setdwunzrad4t7xp@kernel.org>
On 08/08/2017 03:05 PM, Shaohua Li wrote:
>> I'm curious why null_blk isn't a good fit? You'd just need to add RAM
>> storage to it. That would just be a separate option that should be set,
>> ram_backing=1 or something like that. That would make it less critical
>> than using the RAM disk driver as well, since only people that want a "real"
>> data backing would enable it.
>>
>> It's not that I'm extremely opposed to adding a(nother) test block driver,
>> but we at least need some sort of reasoning behind why, which isn't just
>> "not a good fit".
>
> Ah, I thought the 'null' of null_blk means we do nothing for the
> disks. Of course we can rename it, which means this point less
> meaningful. I think the main reason is the interface. We will
> configure the disks with different parameters and do power on/off for
> each disks (which is the key we can emulate disk cache and power
> loss). The module paramter interface of null_blk doesn't work for the
> usage. Of course, these issues can be fixed, for example, we can make
> null_blk use the configfs interface. If you really prefer a single
> driver for all test purpose, I can move the test_blk functionalities
> to null_blk.
The idea with null_blk is just that it's a test vehicle. As such, it
would actually be useful to have a mode where it does store the data in
RAM, since that enables you to do other kinds of testing as well. I'd be
fine with augmenting it with configfs for certain things.
--
Jens Axboe
^ permalink raw reply
* Re: [PATCH 0/5] block: a virtual block device driver for testing
From: Bart Van Assche @ 2017-08-08 22:00 UTC (permalink / raw)
To: shli@kernel.org, axboe@kernel.dk
Cc: linux-raid@vger.kernel.org, kernel-team@fb.com, hare@suse.de,
linux-block@vger.kernel.org, kyungchan.koh@gmail.com, shli@fb.com
In-Reply-To: <73342715-25af-da8e-ae7e-f24015432055@kernel.dk>
On Tue, 2017-08-08 at 15:13 -0600, Jens Axboe wrote:
> On 08/08/2017 03:05 PM, Shaohua Li wrote:
> > > I'm curious why null_blk isn't a good fit? You'd just need to add RAM
> > > storage to it. That would just be a separate option that should be
> > > set,
> > > ram_backing=1 or something like that. That would make it less critical
> > > than using the RAM disk driver as well, since only people that want a
> > > "real"
> > > data backing would enable it.
> > >
> > > It's not that I'm extremely opposed to adding a(nother) test block
> > > driver,
> > > but we at least need some sort of reasoning behind why, which isn't
> > > just
> > > "not a good fit".
> >
> > Ah, I thought the 'null' of null_blk means we do nothing for the
> > disks. Of course we can rename it, which means this point less
> > meaningful. I think the main reason is the interface. We will
> > configure the disks with different parameters and do power on/off for
> > each disks (which is the key we can emulate disk cache and power
> > loss). The module paramter interface of null_blk doesn't work for the
> > usage. Of course, these issues can be fixed, for example, we can make
> > null_blk use the configfs interface. If you really prefer a single
> > driver for all test purpose, I can move the test_blk functionalities
> > to null_blk.
>
> The idea with null_blk is just that it's a test vehicle. As such, it
> would actually be useful to have a mode where it does store the data in
> RAM, since that enables you to do other kinds of testing as well. I'd be
> fine with augmenting it with configfs for certain things.
Hello Jens,
Would you consider it acceptable to make the mode in which null_blk stores
data the default? I know several people who got confused by null_blk by
default not retaining data ...
Thanks,
Bart.
^ permalink raw reply
* Re: [PATCH 0/5] block: a virtual block device driver for testing
From: Jens Axboe @ 2017-08-08 22:07 UTC (permalink / raw)
To: Bart Van Assche, shli@kernel.org
Cc: linux-raid@vger.kernel.org, kernel-team@fb.com, hare@suse.de,
linux-block@vger.kernel.org, kyungchan.koh@gmail.com, shli@fb.com
In-Reply-To: <1502229619.2686.1.camel@wdc.com>
On 08/08/2017 04:00 PM, Bart Van Assche wrote:
> On Tue, 2017-08-08 at 15:13 -0600, Jens Axboe wrote:
>> On 08/08/2017 03:05 PM, Shaohua Li wrote:
>>>> I'm curious why null_blk isn't a good fit? You'd just need to add RAM
>>>> storage to it. That would just be a separate option that should be
>>>> set,
>>>> ram_backing=1 or something like that. That would make it less critical
>>>> than using the RAM disk driver as well, since only people that want a
>>>> "real"
>>>> data backing would enable it.
>>>>
>>>> It's not that I'm extremely opposed to adding a(nother) test block
>>>> driver,
>>>> but we at least need some sort of reasoning behind why, which isn't
>>>> just
>>>> "not a good fit".
>>>
>>> Ah, I thought the 'null' of null_blk means we do nothing for the
>>> disks. Of course we can rename it, which means this point less
>>> meaningful. I think the main reason is the interface. We will
>>> configure the disks with different parameters and do power on/off for
>>> each disks (which is the key we can emulate disk cache and power
>>> loss). The module paramter interface of null_blk doesn't work for the
>>> usage. Of course, these issues can be fixed, for example, we can make
>>> null_blk use the configfs interface. If you really prefer a single
>>> driver for all test purpose, I can move the test_blk functionalities
>>> to null_blk.
>>
>> The idea with null_blk is just that it's a test vehicle. As such, it
>> would actually be useful to have a mode where it does store the data in
>> RAM, since that enables you to do other kinds of testing as well. I'd be
>> fine with augmenting it with configfs for certain things.
>
> Hello Jens,
>
> Would you consider it acceptable to make the mode in which null_blk stores
> data the default? I know several people who got confused by null_blk by
> default not retaining data ...
I don't think we should change the default, since that'll then upset
people that currently use it and all of a sudden see a different
performance profile. It's called null_blk, and the device node is
/dev/nullb0. I think either one of those should reasonably set
expectations for the user that it doesn't really store your data at all.
We could add a module info blurb on it not storing data, I see we don't
have that. The initial commit said:
commit f2298c0403b0dfcaef637eba0c02c4a06d7a25ab
Author: Jens Axboe <axboe@kernel.dk>
Date: Fri Oct 25 11:52:25 2013 +0100
null_blk: multi queue aware block test driver
A driver that simply completes IO it receives, it does no
transfers. Written to fascilitate testing of the blk-mq code.
It supports various module options to use either bio queueing,
rq queueing, or mq mode.
We should just add a MODULE_INFO with a short, similarly worded text.
--
Jens Axboe
^ permalink raw reply
* Re: [PATCH 0/5] block: a virtual block device driver for testing
From: Omar Sandoval @ 2017-08-08 22:08 UTC (permalink / raw)
To: Bart Van Assche
Cc: shli@kernel.org, axboe@kernel.dk, linux-raid@vger.kernel.org,
kernel-team@fb.com, hare@suse.de, linux-block@vger.kernel.org,
kyungchan.koh@gmail.com, shli@fb.com
In-Reply-To: <1502229619.2686.1.camel@wdc.com>
On Tue, Aug 08, 2017 at 10:00:21PM +0000, Bart Van Assche wrote:
> On Tue, 2017-08-08 at 15:13 -0600, Jens Axboe wrote:
> > On 08/08/2017 03:05 PM, Shaohua Li wrote:
> > > > I'm curious why null_blk isn't a good fit? You'd just need to add RAM
> > > > storage to it. That would just be a separate option that should be
> > > > set,
> > > > ram_backing=1 or something like that. That would make it less critical
> > > > than using the RAM disk driver as well, since only people that want a
> > > > "real"
> > > > data backing would enable it.
> > > >
> > > > It's not that I'm extremely opposed to adding a(nother) test block
> > > > driver,
> > > > but we at least need some sort of reasoning behind why, which isn't
> > > > just
> > > > "not a good fit".
> > >
> > > Ah, I thought the 'null' of null_blk means we do nothing for the
> > > disks. Of course we can rename it, which means this point less
> > > meaningful. I think the main reason is the interface. We will
> > > configure the disks with different parameters and do power on/off for
> > > each disks (which is the key we can emulate disk cache and power
> > > loss). The module paramter interface of null_blk doesn't work for the
> > > usage. Of course, these issues can be fixed, for example, we can make
> > > null_blk use the configfs interface. If you really prefer a single
> > > driver for all test purpose, I can move the test_blk functionalities
> > > to null_blk.
> >
> > The idea with null_blk is just that it's a test vehicle. As such, it
> > would actually be useful to have a mode where it does store the data in
> > RAM, since that enables you to do other kinds of testing as well. I'd be
> > fine with augmenting it with configfs for certain things.
>
> Hello Jens,
>
> Would you consider it acceptable to make the mode in which null_blk stores
> data the default? I know several people who got confused by null_blk by
> default not retaining data ...
My minor issue with that is that I'd have to change all of my
performance testing scripts to override the default :) I'd prefer that
it stay as-is.
^ permalink raw reply
* force raid reassembly
From: Bryan Hepworth @ 2017-08-08 22:27 UTC (permalink / raw)
To: linux-raid@vger.kernel.org
In-Reply-To: <DB4PR07MB409B56E9B9173E33079902ED58A0@DB4PR07MB409.eurprd07.prod.outlook.com>
Hi all
I'd be grateful of another pair of eyes just to check and verify what I'm thinking. I've had an array fail and I've checked the components with the mdadm --examine command. Five out of the six event counts are correct and I'm hoping I can get it to assemble with the force option: -
"mdadm --assemble --force /dev/mdX <list of devices>"
mdadm --assemble --force /dev/md0 /dev/sdd1 /dev/sdg1 /dev/sdf1 /dev/sde1 /dev/sdb1 /dev/sdc1
Output from --examine is as follows: -
[root@inscribe4 dev]# mdadm --examine /dev/sde1
/dev/sde1:
Magic : a92b4efc
Version : 1.2
Feature Map : 0x1
Array UUID : ace00fb5:8771d55a:cf36728b:ddc3ce89
Name : intermediate0
Creation Time : Tue Oct 15 21:03:50 2013
Raid Level : raid5
Raid Devices : 6
Avail Dev Size : 3906761858 (1862.89 GiB 2000.26 GB)
Array Size : 9766901760 (9314.44 GiB 10001.31 GB)
Used Dev Size : 3906760704 (1862.89 GiB 2000.26 GB)
Data Offset : 262144 sectors
Super Offset : 8 sectors
Unused Space : before=262064 sectors, after=1154 sectors
State : clean
Device UUID : 8d0b6c8e:0c526265:6ec0a71b:52a636ba
Internal Bitmap : 8 sectors from superblock
Update Time : Tue Aug 8 14:16:53 2017
Checksum : 44b7b461 - correct
Events : 119894
Layout : left-symmetric
Chunk Size : 512K
Device Role : spare
Array State : AAA.A. ('A' == active, '.' == missing, 'R' == replacing)
[root@inscribe4 dev]# mdadm --examine /dev/sdg1
/dev/sdg1:
Magic : a92b4efc
Version : 1.2
Feature Map : 0x1
Array UUID : ace00fb5:8771d55a:cf36728b:ddc3ce89
Name : intermediate0
Creation Time : Tue Oct 15 21:03:50 2013
Raid Level : raid5
Raid Devices : 6
Avail Dev Size : 3906761858 (1862.89 GiB 2000.26 GB)
Array Size : 9766901760 (9314.44 GiB 10001.31 GB)
Used Dev Size : 3906760704 (1862.89 GiB 2000.26 GB)
Data Offset : 262144 sectors
Super Offset : 8 sectors
Unused Space : before=262064 sectors, after=1154 sectors
State : clean
Device UUID : e238e74b:d40efc43:d34d876d:ca0700b0
Internal Bitmap : 8 sectors from superblock
Update Time : Tue Aug 8 14:16:53 2017
Checksum : 28758d54 - correct
Events : 119894
Layout : left-symmetric
Chunk Size : 512K
Device Role : Active device 1
Array State : AAA.A. ('A' == active, '.' == missing, 'R' == replacing)
[root@inscribe4 dev]# mdadm --examine /dev/sdd1
/dev/sdd1:
Magic : a92b4efc
Version : 1.2
Feature Map : 0x1
Array UUID : ace00fb5:8771d55a:cf36728b:ddc3ce89
Name : intermediate0
Creation Time : Tue Oct 15 21:03:50 2013
Raid Level : raid5
Raid Devices : 6
Avail Dev Size : 3906761858 (1862.89 GiB 2000.26 GB)
Array Size : 9766901760 (9314.44 GiB 10001.31 GB)
Used Dev Size : 3906760704 (1862.89 GiB 2000.26 GB)
Data Offset : 262144 sectors
Super Offset : 8 sectors
Unused Space : before=262064 sectors, after=1154 sectors
State : clean
Device UUID : 1732f121:f6f0df09:fafd2411:7c78ce00
Internal Bitmap : 8 sectors from superblock
Update Time : Tue Aug 8 14:16:53 2017
Checksum : b8cf8981 - correct
Events : 119894
Layout : left-symmetric
Chunk Size : 512K
Device Role : Active device 0
Array State : AAA.A. ('A' == active, '.' == missing, 'R' == replacing)
[root@inscribe4 dev]# mdadm --examine /dev/sdf1
/dev/sdf1:
Magic : a92b4efc
Version : 1.2
Feature Map : 0x1
Array UUID : ace00fb5:8771d55a:cf36728b:ddc3ce89
Name : intermediate0
Creation Time : Tue Oct 15 21:03:50 2013
Raid Level : raid5
Raid Devices : 6
Avail Dev Size : 3906761858 (1862.89 GiB 2000.26 GB)
Array Size : 9766901760 (9314.44 GiB 10001.31 GB)
Used Dev Size : 3906760704 (1862.89 GiB 2000.26 GB)
Data Offset : 262144 sectors
Super Offset : 8 sectors
Unused Space : before=262064 sectors, after=1154 sectors
State : clean
Device UUID : 3d414ad2:4310c1f9:98f50214:3e9b1e33
Internal Bitmap : 8 sectors from superblock
Update Time : Tue Aug 8 14:16:53 2017
Checksum : 8e37d25d - correct
Events : 119894
Layout : left-symmetric
Chunk Size : 512K
Device Role : Active device 2
Array State : AAA.A. ('A' == active, '.' == missing, 'R' == replacing)
[root@inscribe4 dev]# mdadm --examine /dev/sdb1
/dev/sdb1:
Magic : a92b4efc
Version : 1.2
Feature Map : 0x1
Array UUID : ace00fb5:8771d55a:cf36728b:ddc3ce89
Name : intermediate0
Creation Time : Tue Oct 15 21:03:50 2013
Raid Level : raid5
Raid Devices : 6
Avail Dev Size : 3906761858 (1862.89 GiB 2000.26 GB)
Array Size : 9766901760 (9314.44 GiB 10001.31 GB)
Used Dev Size : 3906760704 (1862.89 GiB 2000.26 GB)
Data Offset : 262144 sectors
Super Offset : 8 sectors
Unused Space : before=262064 sectors, after=1154 sectors
State : clean
Device UUID : a84263cd:75ce20b9:a3a72a1c:b18b2457
Internal Bitmap : 8 sectors from superblock
Update Time : Mon Jul 31 22:50:37 2017
Checksum : 74d320d6 - correct
Events : 119879
Layout : left-symmetric
Chunk Size : 512K
Device Role : Active device 5
Array State : AAA.AA ('A' == active, '.' == missing, 'R' == replacing)
[root@inscribe4 dev]# mdadm --examine /dev/sdc1
/dev/sdc1:
Magic : a92b4efc
Version : 1.2
Feature Map : 0x1
Array UUID : ace00fb5:8771d55a:cf36728b:ddc3ce89
Name : intermediate0
Creation Time : Tue Oct 15 21:03:50 2013
Raid Level : raid5
Raid Devices : 6
Avail Dev Size : 3906761858 (1862.89 GiB 2000.26 GB)
Array Size : 9766901760 (9314.44 GiB 10001.31 GB)
Used Dev Size : 3906760704 (1862.89 GiB 2000.26 GB)
Data Offset : 262144 sectors
Super Offset : 8 sectors
Unused Space : before=262064 sectors, after=1154 sectors
State : clean
Device UUID : be5ec20e:249645e5:22aeca25:a60c7599
Internal Bitmap : 8 sectors from superblock
Update Time : Tue Aug 8 14:16:53 2017
Checksum : 2e529fae - correct
Events : 119894
Layout : left-symmetric
Chunk Size : 512K
Device Role : Active device 4
Array State : AAA.A. ('A' == active, '.' == missing, 'R' == replacing)
Thanks
Bryan
^ permalink raw reply
* Re: force raid reassembly
From: Joe Landman @ 2017-08-08 22:44 UTC (permalink / raw)
To: Bryan Hepworth, linux-raid@vger.kernel.org
In-Reply-To: <WM!bafd27028d31e7e922c12b615def50baa8369ba598ac220b2884f1706eddac34cd0580e2d54cc3da9be1bbadf104dab1!@mailhub-mx1.ncl.ac.uk>
On 8/8/17 3:27 PM, Bryan Hepworth wrote:
> Hi all
>
> I'd be grateful of another pair of eyes just to check and verify what I'm thinking. I've had an array fail and I've checked the components with the mdadm --examine command. Five out of the six event counts are correct and I'm hoping I can get it to assemble with the force option: -
>
> "mdadm --assemble --force /dev/mdX <list of devices>"
>
> mdadm --assemble --force /dev/md0 /dev/sdd1 /dev/sdg1 /dev/sdf1 /dev/sde1 /dev/sdb1 /dev/sdc1
I don't think you need to explicitly list the devices (/dev/sd(..)) in
the force
You could do something like this:
mdadm --examine --scan | grep md0 > /tmp/md0.conf
mdadm -As -c /tmp/md0.conf
if this doesn't work, then the force *may* be an option, but be very
careful with this.
>
> Output from --examine is as follows: -
>
>
> [root@inscribe4 dev]# mdadm --examine /dev/sde1
> /dev/sde1:
> Magic : a92b4efc
> Version : 1.2
> Feature Map : 0x1
> Array UUID : ace00fb5:8771d55a:cf36728b:ddc3ce89
> Name : intermediate0
> Creation Time : Tue Oct 15 21:03:50 2013
> Raid Level : raid5
> Raid Devices : 6
>
> Avail Dev Size : 3906761858 (1862.89 GiB 2000.26 GB)
> Array Size : 9766901760 (9314.44 GiB 10001.31 GB)
> Used Dev Size : 3906760704 (1862.89 GiB 2000.26 GB)
> Data Offset : 262144 sectors
> Super Offset : 8 sectors
> Unused Space : before=262064 sectors, after=1154 sectors
> State : clean
> Device UUID : 8d0b6c8e:0c526265:6ec0a71b:52a636ba
>
> Internal Bitmap : 8 sectors from superblock
> Update Time : Tue Aug 8 14:16:53 2017
> Checksum : 44b7b461 - correct
> Events : 119894
>
> Layout : left-symmetric
> Chunk Size : 512K
>
> Device Role : spare
> Array State : AAA.A. ('A' == active, '.' == missing, 'R' == replacing)
>
> [root@inscribe4 dev]# mdadm --examine /dev/sdg1
> /dev/sdg1:
> Magic : a92b4efc
> Version : 1.2
> Feature Map : 0x1
> Array UUID : ace00fb5:8771d55a:cf36728b:ddc3ce89
> Name : intermediate0
> Creation Time : Tue Oct 15 21:03:50 2013
> Raid Level : raid5
> Raid Devices : 6
>
> Avail Dev Size : 3906761858 (1862.89 GiB 2000.26 GB)
> Array Size : 9766901760 (9314.44 GiB 10001.31 GB)
> Used Dev Size : 3906760704 (1862.89 GiB 2000.26 GB)
> Data Offset : 262144 sectors
> Super Offset : 8 sectors
> Unused Space : before=262064 sectors, after=1154 sectors
> State : clean
> Device UUID : e238e74b:d40efc43:d34d876d:ca0700b0
>
> Internal Bitmap : 8 sectors from superblock
> Update Time : Tue Aug 8 14:16:53 2017
> Checksum : 28758d54 - correct
> Events : 119894
>
> Layout : left-symmetric
> Chunk Size : 512K
>
> Device Role : Active device 1
> Array State : AAA.A. ('A' == active, '.' == missing, 'R' == replacing)
>
> [root@inscribe4 dev]# mdadm --examine /dev/sdd1
> /dev/sdd1:
> Magic : a92b4efc
> Version : 1.2
> Feature Map : 0x1
> Array UUID : ace00fb5:8771d55a:cf36728b:ddc3ce89
> Name : intermediate0
> Creation Time : Tue Oct 15 21:03:50 2013
> Raid Level : raid5
> Raid Devices : 6
>
> Avail Dev Size : 3906761858 (1862.89 GiB 2000.26 GB)
> Array Size : 9766901760 (9314.44 GiB 10001.31 GB)
> Used Dev Size : 3906760704 (1862.89 GiB 2000.26 GB)
> Data Offset : 262144 sectors
> Super Offset : 8 sectors
> Unused Space : before=262064 sectors, after=1154 sectors
> State : clean
> Device UUID : 1732f121:f6f0df09:fafd2411:7c78ce00
>
> Internal Bitmap : 8 sectors from superblock
> Update Time : Tue Aug 8 14:16:53 2017
> Checksum : b8cf8981 - correct
> Events : 119894
>
> Layout : left-symmetric
> Chunk Size : 512K
>
> Device Role : Active device 0
> Array State : AAA.A. ('A' == active, '.' == missing, 'R' == replacing)
>
> [root@inscribe4 dev]# mdadm --examine /dev/sdf1
> /dev/sdf1:
> Magic : a92b4efc
> Version : 1.2
> Feature Map : 0x1
> Array UUID : ace00fb5:8771d55a:cf36728b:ddc3ce89
> Name : intermediate0
> Creation Time : Tue Oct 15 21:03:50 2013
> Raid Level : raid5
> Raid Devices : 6
>
> Avail Dev Size : 3906761858 (1862.89 GiB 2000.26 GB)
> Array Size : 9766901760 (9314.44 GiB 10001.31 GB)
> Used Dev Size : 3906760704 (1862.89 GiB 2000.26 GB)
> Data Offset : 262144 sectors
> Super Offset : 8 sectors
> Unused Space : before=262064 sectors, after=1154 sectors
> State : clean
> Device UUID : 3d414ad2:4310c1f9:98f50214:3e9b1e33
>
> Internal Bitmap : 8 sectors from superblock
> Update Time : Tue Aug 8 14:16:53 2017
> Checksum : 8e37d25d - correct
> Events : 119894
>
> Layout : left-symmetric
> Chunk Size : 512K
>
> Device Role : Active device 2
> Array State : AAA.A. ('A' == active, '.' == missing, 'R' == replacing)
>
> [root@inscribe4 dev]# mdadm --examine /dev/sdb1
> /dev/sdb1:
> Magic : a92b4efc
> Version : 1.2
> Feature Map : 0x1
> Array UUID : ace00fb5:8771d55a:cf36728b:ddc3ce89
> Name : intermediate0
> Creation Time : Tue Oct 15 21:03:50 2013
> Raid Level : raid5
> Raid Devices : 6
>
> Avail Dev Size : 3906761858 (1862.89 GiB 2000.26 GB)
> Array Size : 9766901760 (9314.44 GiB 10001.31 GB)
> Used Dev Size : 3906760704 (1862.89 GiB 2000.26 GB)
> Data Offset : 262144 sectors
> Super Offset : 8 sectors
> Unused Space : before=262064 sectors, after=1154 sectors
> State : clean
> Device UUID : a84263cd:75ce20b9:a3a72a1c:b18b2457
>
> Internal Bitmap : 8 sectors from superblock
> Update Time : Mon Jul 31 22:50:37 2017
> Checksum : 74d320d6 - correct
> Events : 119879
>
> Layout : left-symmetric
> Chunk Size : 512K
>
> Device Role : Active device 5
> Array State : AAA.AA ('A' == active, '.' == missing, 'R' == replacing)
>
> [root@inscribe4 dev]# mdadm --examine /dev/sdc1
> /dev/sdc1:
> Magic : a92b4efc
> Version : 1.2
> Feature Map : 0x1
> Array UUID : ace00fb5:8771d55a:cf36728b:ddc3ce89
> Name : intermediate0
> Creation Time : Tue Oct 15 21:03:50 2013
> Raid Level : raid5
> Raid Devices : 6
>
> Avail Dev Size : 3906761858 (1862.89 GiB 2000.26 GB)
> Array Size : 9766901760 (9314.44 GiB 10001.31 GB)
> Used Dev Size : 3906760704 (1862.89 GiB 2000.26 GB)
> Data Offset : 262144 sectors
> Super Offset : 8 sectors
> Unused Space : before=262064 sectors, after=1154 sectors
> State : clean
> Device UUID : be5ec20e:249645e5:22aeca25:a60c7599
>
> Internal Bitmap : 8 sectors from superblock
> Update Time : Tue Aug 8 14:16:53 2017
> Checksum : 2e529fae - correct
> Events : 119894
>
> Layout : left-symmetric
> Chunk Size : 512K
>
> Device Role : Active device 4
> Array State : AAA.A. ('A' == active, '.' == missing, 'R' == replacing)
>
> Thanks
>
> Bryan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Joe Landman
e: joe.landman@gmail.com
t: @hpcjoe
c: +1 734 612 4615
w: https://scalability.org
g: https://github.com/joelandman
l: https://www.linkedin.com/in/joelandman
^ permalink raw reply
* [PATCH] md/r5cache: call mddev_lock/unlock() in r5c_journal_mode_show
From: Song Liu @ 2017-08-08 22:54 UTC (permalink / raw)
To: linux-raid, shli
Cc: Song Liu, shli, neilb, kernel-team, dan.j.williams, hch,
jes.sorensen
In r5c_journal_mode_show(), it is necessary to call mddev_lock()
before accessing conf and conf->log. Otherwise, the conf->log
may change (and become NULL).
Signed-off-by: Song Liu <songliubraving@fb.com>
---
drivers/md/raid5-cache.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index 2dcbafa..88d5432 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -2529,11 +2529,15 @@ static void r5l_write_super(struct r5l_log *log, sector_t cp)
static ssize_t r5c_journal_mode_show(struct mddev *mddev, char *page)
{
- struct r5conf *conf = mddev->private;
+ struct r5conf *conf;
int ret;
- if (!conf->log)
+ mddev_lock(mddev);
+ conf = mddev->private;
+ if (!conf || !conf->log) {
+ mddev_unlock(mddev);
return 0;
+ }
switch (conf->log->r5c_journal_mode) {
case R5C_JOURNAL_MODE_WRITE_THROUGH:
@@ -2551,6 +2555,7 @@ static ssize_t r5c_journal_mode_show(struct mddev *mddev, char *page)
default:
ret = 0;
}
+ mddev_unlock(mddev);
return ret;
}
--
2.9.3
^ permalink raw reply related
* Re: [PATCH] Assemble: prevent segfault with faulty "best" devices
From: NeilBrown @ 2017-08-09 0:46 UTC (permalink / raw)
To: Andrea Righi, Jes Sorensen; +Cc: Robert LeBlanc, linux-raid
In-Reply-To: <20170808174807.GA6611@Dell>
[-- Attachment #1: Type: text/plain, Size: 8978 bytes --]
On Tue, Aug 08 2017, Andrea Righi wrote:
> I was able to trigger this curious problem that seems to happen only on
> one of our server:
>
> # mdadm --assemble /dev/md/10.4.237.12-volume --name 10.4.237.12-volume
> Segmentation fault
>
> This md volume is a raid1 volume made of 2 device mapper (dm-multipath)
> devices and the underlying LUNs are imported via iSCSI.
>
> Applying the following patch (see below) seems to fix the problem:
>
> # ./mdadm --assemble /dev/md/10.4.237.12-volume --name 10.4.237.12-volume
> mdadm: /dev/md/10.4.237.12-volume has been started with 2 drives.
>
> But I'm not sure if it's the right fix or if there're some other
> problems that I'm missing.
>
> More details about the md superblocks that might help to better
> understand the nature of the problem:
>
> # for i in 36001405a04ed0c104881{1,2}00000000000p2; do echo dev: ${i}; mdadm --examine /dev/mapper/${i}; done
> dev: 36001405a04ed0c104881100000000000p2
> /dev/mapper/36001405a04ed0c104881100000000000p2:
> Magic : a92b4efc
> Version : 1.2
> Feature Map : 0x1
> Array UUID : 5f3e8283:7f831b85:bc1958b9:6f2787a4
> Name : 10.4.237.12-volume
> Creation Time : Thu Jul 27 14:43:16 2017
> Raid Level : raid1
> Raid Devices : 2
>
> Avail Dev Size : 1073729503 (511.99 GiB 549.75 GB)
> Array Size : 536864704 (511.99 GiB 549.75 GB)
> Used Dev Size : 1073729408 (511.99 GiB 549.75 GB)
> Data Offset : 8192 sectors
> Super Offset : 8 sectors
> Unused Space : before=8104 sectors, after=95 sectors
> State : clean
> Device UUID : 16dae7e3:42f3487f:fbeac43a:71cf1f63
>
> Internal Bitmap : 8 sectors from superblock
> Update Time : Tue Aug 8 11:12:22 2017
> Bad Block Log : 512 entries available at offset 72 sectors
> Checksum : 518c443e - correct
> Events : 167
>
>
> Device Role : Active device 0
> Array State : AA ('A' == active, '.' == missing, 'R' == replacing)
> dev: 36001405a04ed0c104881200000000000p2
> /dev/mapper/36001405a04ed0c104881200000000000p2:
> Magic : a92b4efc
> Version : 1.2
> Feature Map : 0x1
> Array UUID : 5f3e8283:7f831b85:bc1958b9:6f2787a4
> Name : 10.4.237.12-volume
> Creation Time : Thu Jul 27 14:43:16 2017
> Raid Level : raid1
> Raid Devices : 2
>
> Avail Dev Size : 1073729503 (511.99 GiB 549.75 GB)
> Array Size : 536864704 (511.99 GiB 549.75 GB)
> Used Dev Size : 1073729408 (511.99 GiB 549.75 GB)
> Data Offset : 8192 sectors
> Super Offset : 8 sectors
> Unused Space : before=8104 sectors, after=95 sectors
> State : clean
> Device UUID : ef612bdd:e475fe02:5d3fc55e:53612f34
>
> Internal Bitmap : 8 sectors from superblock
> Update Time : Tue Aug 8 11:12:22 2017
> Bad Block Log : 512 entries available at offset 72 sectors
> Checksum : c39534fd - correct
> Events : 167
>
>
> Device Role : Active device 1
> Array State : AA ('A' == active, '.' == missing, 'R' == replacing)
>
> # for i in 36001405a04ed0c104881{1,2}00000000000p2; do echo dev: ${i}; hexdump -s 4096 -n 4189696 -C /dev/mapper/${i}; done
> dev: 36001405a04ed0c104881100000000000p2
> 00001000 fc 4e 2b a9 01 00 00 00 01 00 00 00 00 00 00 00 |.N+.............|
> 00001010 5f 3e 82 83 7f 83 1b 85 bc 19 58 b9 6f 27 87 a4 |_>........X.o'..|
> 00001020 31 30 2e 34 2e 32 33 37 2e 31 32 2d 76 6f 6c 75 |10.4.237.12-volu|
> 00001030 6d 65 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |me..............|
> 00001040 64 50 7a 59 00 00 00 00 01 00 00 00 00 00 00 00 |dPzY............|
> 00001050 80 cf ff 3f 00 00 00 00 00 00 00 00 02 00 00 00 |...?............|
> 00001060 08 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> 00001070 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> 00001080 00 20 00 00 00 00 00 00 df cf ff 3f 00 00 00 00 |. .........?....|
> 00001090 08 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> 000010a0 00 00 00 00 00 00 00 00 16 da e7 e3 42 f3 48 7f |............B.H.|
> 000010b0 fb ea c4 3a 71 cf 1f 63 00 00 08 00 48 00 00 00 |...:q..c....H...|
> 000010c0 54 f0 89 59 00 00 00 00 a7 00 00 00 00 00 00 00 |T..Y............|
> 000010d0 ff ff ff ff ff ff ff ff 9c 43 8c 51 80 00 00 00 |.........C.Q....|
> 000010e0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> *
> 00001100 00 00 01 00 fe ff fe ff fe ff fe ff fe ff fe ff |................|
> 00001110 fe ff fe ff fe ff fe ff fe ff fe ff fe ff fe ff |................|
> *
> 00001200 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> *
> 00002000 62 69 74 6d 04 00 00 00 5f 3e 82 83 7f 83 1b 85 |bitm...._>......|
> 00002010 bc 19 58 b9 6f 27 87 a4 a7 00 00 00 00 00 00 00 |..X.o'..........|
> 00002020 a7 00 00 00 00 00 00 00 80 cf ff 3f 00 00 00 00 |...........?....|
> 00002030 00 00 00 00 00 00 00 01 05 00 00 00 00 00 00 00 |................|
> 00002040 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> *
> 00003100 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
> *
> 00004000 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> *
> 003ffe00
> dev: 36001405a04ed0c104881200000000000p2
> 00001000 fc 4e 2b a9 01 00 00 00 01 00 00 00 00 00 00 00 |.N+.............|
> 00001010 5f 3e 82 83 7f 83 1b 85 bc 19 58 b9 6f 27 87 a4 |_>........X.o'..|
> 00001020 31 30 2e 34 2e 32 33 37 2e 31 32 2d 76 6f 6c 75 |10.4.237.12-volu|
> 00001030 6d 65 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |me..............|
> 00001040 64 50 7a 59 00 00 00 00 01 00 00 00 00 00 00 00 |dPzY............|
> 00001050 80 cf ff 3f 00 00 00 00 00 00 00 00 02 00 00 00 |...?............|
> 00001060 08 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> 00001070 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> 00001080 00 20 00 00 00 00 00 00 df cf ff 3f 00 00 00 00 |. .........?....|
> 00001090 08 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> 000010a0 01 00 00 00 00 00 00 00 ef 61 2b dd e4 75 fe 02 |.........a+..u..|
> 000010b0 5d 3f c5 5e 53 61 2f 34 00 00 08 00 48 00 00 00 |]?.^Sa/4....H...|
> 000010c0 54 f0 89 59 00 00 00 00 a7 00 00 00 00 00 00 00 |T..Y............|
> 000010d0 ff ff ff ff ff ff ff ff 5b 34 95 c3 80 00 00 00 |........[4......|
> 000010e0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> *
> 00001100 00 00 01 00 fe ff fe ff fe ff fe ff fe ff fe ff |................|
> 00001110 fe ff fe ff fe ff fe ff fe ff fe ff fe ff fe ff |................|
> *
> 00001200 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> *
> 00002000 62 69 74 6d 04 00 00 00 5f 3e 82 83 7f 83 1b 85 |bitm...._>......|
> 00002010 bc 19 58 b9 6f 27 87 a4 a7 00 00 00 00 00 00 00 |..X.o'..........|
> 00002020 a7 00 00 00 00 00 00 00 80 cf ff 3f 00 00 00 00 |...........?....|
> 00002030 00 00 00 00 00 00 00 01 05 00 00 00 00 00 00 00 |................|
> 00002040 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> *
> 00003100 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
> *
> 00004000 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
> *
> 003ffe00
>
> ---
> Assemble: prevent segfault with faulty "best" devices
>
> In Assemble(), after context reload, best[i] can be -1 in some cases,
> and before checking if this value is negative we use it to access
> devices[j].i.disk.raid_disk, potentially causing a segfault.
>
> Check if best[i] is negative before using it to prevent this potential
> segfault.
>
> Signed-off-by: Andrea Righi <andrea@betterlinux.com>
> Signed-off-by: Robert LeBlanc <robert@leblancnet.us>
> ---
> Assemble.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Assemble.c b/Assemble.c
> index 3da0903..fc681eb 100644
> --- a/Assemble.c
> +++ b/Assemble.c
> @@ -1669,6 +1669,8 @@ try_again:
> int j = best[i];
> unsigned int desired_state;
>
> + if (j < 0)
> + continue;
> if (devices[j].i.disk.raid_disk == MD_DISK_ROLE_JOURNAL)
> desired_state = (1<<MD_DISK_JOURNAL);
> else if (i >= content->array.raid_disks * 2)
> @@ -1678,8 +1680,6 @@ try_again:
> else
> desired_state = (1<<MD_DISK_ACTIVE) | (1<<MD_DISK_SYNC);
>
> - if (j<0)
> - continue;
> if (!devices[j].uptodate)
> continue;
>
Patch looks good to me, thanks.
Regression was causes by commit 69a481166be6 ("Assemble array with write
journal") which introduced a use of 'j' before the test if it was < 0.
Fixes: 69a481166be6 ("Assemble array with write journal")
Reviewed-by: NeilBrown <neilb@suse.com>
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* Re: [PATCH v4 02/19] crypto: ccm: use -EAGAIN for transient busy indication
From: Gilad Ben-Yossef @ 2017-08-09 5:18 UTC (permalink / raw)
To: Gary R Hook
Cc: Herbert Xu, David S. Miller, Jonathan Corbet, David Howells,
Lendacky, Thomas, Boris Brezillon, Arnaud Ebalard,
Matthias Brugger, Alasdair Kergon, Mike Snitzer,
dm-devel@redhat.com, Shaohua Li, Steve French,
Theodore Y. Ts'o, Jaegeuk Kim, Mimi Zohar, Dmitry Kasatkin,
James Morris, Serge E. Hallyn, linux-
In-Reply-To: <4600c898-a2f2-f95f-15b4-0adcc6acc0b5@amd.com>
On Tue, Aug 8, 2017 at 6:24 PM, Gary R Hook <gary.hook@amd.com> wrote:
> On 08/08/2017 07:03 AM, Gilad Ben-Yossef wrote:
>>
>> Replace -EBUSY with -EAGAIN when reporting transient busy
>> indication in the absence of backlog.
>>
>> Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
>
>
> Could we use "ccp" in the subject line, please?
Of course, sorry about the the typo.
Gilad
--
Gilad Ben-Yossef
Chief Coffee Drinker
"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
-- Jean-Baptiste Queru
^ permalink raw reply
* [PATCH v2] md/r5cache: call mddev_lock/unlock() in r5c_journal_mode_show
From: Song Liu @ 2017-08-09 5:56 UTC (permalink / raw)
To: linux-raid, shli
Cc: fengguang.wu, sfr, Song Liu, shli, neilb, kernel-team,
dan.j.williams, hch, jes.sorensen
In r5c_journal_mode_show(), it is necessary to call mddev_lock()
before accessing conf and conf->log. Otherwise, the conf->log
may change (and become NULL).
Signed-off-by: Song Liu <songliubraving@fb.com>
Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Reported-by: kbuild test robot <fengguang.wu@intel.com>
---
drivers/md/raid5-cache.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index 2dcbafa..b6f3702 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -2529,11 +2529,19 @@ static void r5l_write_super(struct r5l_log *log, sector_t cp)
static ssize_t r5c_journal_mode_show(struct mddev *mddev, char *page)
{
- struct r5conf *conf = mddev->private;
+ struct r5conf *conf;
int ret;
+ int err;
- if (!conf->log)
+ err = mddev_lock(mddev);
+ if (err)
+ return err;
+
+ conf = mddev->private;
+ if (!conf || !conf->log) {
+ mddev_unlock(mddev);
return 0;
+ }
switch (conf->log->r5c_journal_mode) {
case R5C_JOURNAL_MODE_WRITE_THROUGH:
@@ -2551,6 +2559,7 @@ static ssize_t r5c_journal_mode_show(struct mddev *mddev, char *page)
default:
ret = 0;
}
+ mddev_unlock(mddev);
return ret;
}
--
2.9.3
^ permalink raw reply related
* Re: [MD] Crash with 4.12+ kernel and high disk load -- bisected to 4ad23a976413: MD: use per-cpu counter for writes_pending
From: David R @ 2017-08-09 6:28 UTC (permalink / raw)
To: Dominik Brodowski; +Cc: NeilBrown, Shaohua Li, linux-kernel, linux-raid, tj
In-Reply-To: <20170808090600.GA28583@isilmar-4.linta.de>
My raid check also ran through cleanly, so feel free to add my Tested-By:
Cheers
David
Quoting Dominik Brodowski <linux@dominikbrodowski.net>:
> On Tue, Aug 08, 2017 at 09:36:14AM +0200, Dominik Brodowski wrote:
>> On Tue, Aug 08, 2017 at 05:01:28PM +1000, NeilBrown wrote:
>> > On Mon, Aug 07 2017, Dominik Brodowski wrote:
>> >
>> > > Neil, Shaohua,
>> > >
>> > > following up on David R's bug message: I have observed something similar
>> > > on v4.12.[345] and v4.13-rc4, but not on v4.11. This is a RAID1 (on bare
>> > > metal partitions, /dev/sdaX and /dev/sdbY linked together). In case it
>> > > matters: Further upwards are cryptsetup, a DM volume group, then logical
>> > > volumes, and then filesystems (ext4, but also happened with xfs).
>> > >
>> > > In a tedious bisect (the bug wasn't as quickly reproducible as
>> I would like,
>> > > but happened when I repeatedly created large lvs and filled
>> them with some
>> > > content, while compiling kernels in parallel), I was able to track this
>> > > down to:
>> > >
>> > >
>> > > commit 4ad23a976413aa57fe5ba7a25953dc35ccca5b71
>> > > Author: NeilBrown <neilb@suse.com>
>> > > Date: Wed Mar 15 14:05:14 2017 +1100
>> > >
>> > > MD: use per-cpu counter for writes_pending
>> > >
>> > > The 'writes_pending' counter is used to determine when the
>> > > array is stable so that it can be marked in the superblock
>> > > as "Clean". Consequently it needs to be updated frequently
>> > > but only checked for zero occasionally. Recent changes to
>> > > raid5 cause the count to be updated even more often - once
>> > > per 4K rather than once per bio. This provided
>> > > justification for making the updates more efficient.
>> > >
>> > > ...
>> >
>> > Thanks for the report... and for bisecting and for re-sending...
>> >
>> > I believe I have found the problem, and have sent a patch separately.
>> >
>> > If mddev->safemode == 1 and mddev->in_sync != 0, md_check_recovery()
>> > causes the thread that calls it to spin.
>> > Prior to the patch you found, that couldn't happen. Now it can,
>> > so it needs to be handled more carefully.
>> >
>> > While I was examining the code, I found another bug - so that is a win!
>> >
>> > Thanks,
>> > NeilBrown
>>
>>
>> Nice catch. Thanks! Will give it (both patches at once) a try on the test
>> system immediately.
>
> More than 2 hours of stress-testing shows no issues any more. Very nice.
> Thanks!
>
> Dominik
^ permalink raw reply
* Re: [RESEND PATCH] bcache: Don't reinvent the wheel but use existing llist API
From: Nikolay Borisov @ 2017-08-09 6:39 UTC (permalink / raw)
To: Byungchul Park, Coly Li
Cc: kent.overstreet, shli, linux-bcache, linux-raid, linux-kernel,
kernel-team
In-Reply-To: <20170808060056.GS20323@X58A-UD3R>
On 8.08.2017 09:00, Byungchul Park wrote:
> On Tue, Aug 08, 2017 at 01:28:39PM +0800, Coly Li wrote:
>>>>> + llist_for_each_entry_safe(cl, t, reverse, list) {
>>>>
>>>> Just wondering why not using llist_for_each_entry(), or you use the
>>>> _safe version on purpose ?
>>>
>>> If I use llist_for_each_entry(), then it would change the original
>>> behavior. Is it ok?
Generally, _safe versions of list primitives is used when you are going
to perform removal in the iteration. I haven't looked at the code in
bcache but if it's removing entries from the list then _safe version is
required. If you are only iterating - then non-safe version is fine.
>>>
>>
>> I feel llist_for_each_entry() keeps the original behavior, and variable
>
> Ah.. I see. Then.. Can I change it into non-safe version? Is it still ok
> with non-safe one? I will change it at the next spin, if yes.
>
>> 't' can be removed. Anyway, either llist_for_each_entry() or
>> llist_for_each_entry_safe() works correctly and well here. Any one you
>> use is OK to me, thanks for your informative reply :-)
>
> I rather appriciate it.
>
> Thank you,
> Byungchul
>
^ permalink raw reply
* Re: [RESEND PATCH] bcache: Don't reinvent the wheel but use existing llist API
From: Byungchul Park @ 2017-08-09 6:42 UTC (permalink / raw)
To: Nikolay Borisov
Cc: Coly Li, kent.overstreet, shli, linux-bcache, linux-raid,
linux-kernel, kernel-team
In-Reply-To: <aa85a3ed-d9a0-e9d6-58e2-b6c9e4a63984@suse.com>
On Wed, Aug 09, 2017 at 09:39:09AM +0300, Nikolay Borisov wrote:
>
>
> On 8.08.2017 09:00, Byungchul Park wrote:
> > On Tue, Aug 08, 2017 at 01:28:39PM +0800, Coly Li wrote:
> >>>>> + llist_for_each_entry_safe(cl, t, reverse, list) {
> >>>>
> >>>> Just wondering why not using llist_for_each_entry(), or you use the
> >>>> _safe version on purpose ?
> >>>
> >>> If I use llist_for_each_entry(), then it would change the original
> >>> behavior. Is it ok?
>
> Generally, _safe versions of list primitives is used when you are going
> to perform removal in the iteration. I haven't looked at the code in
> bcache but if it's removing entries from the list then _safe version is
> required. If you are only iterating - then non-safe version is fine.
Thank you~ :)
^ permalink raw reply
* RE: force raid reassembly
From: Bryan Hepworth @ 2017-08-09 7:40 UTC (permalink / raw)
To: Joe Landman, linux-raid@vger.kernel.org
In-Reply-To: <WM!c7e5d36941a99d370e99c9ecff040c34199c6ab48c7c1548590a2e9075a62b0e4a8a44d8c43afabb53f77be4c9108e21!@mailhub-mx3.ncl.ac.uk>
>-----Original Message-----
>From: Joe Landman [mailto:joe.landman@gmail.com]
>Sent: 08 August 2017 23:44
>To: Bryan Hepworth; linux-raid@vger.kernel.org
>Subject: Re: force raid reassembly
>
>
>
>On 8/8/17 3:27 PM, Bryan Hepworth wrote:
>> Hi all
>>
>> I'd be grateful of another pair of eyes just to check and verify what I'm
>thinking. I've had an array fail and I've checked the components with the
>mdadm --examine command. Five out of the six event counts are correct and
>I'm hoping I can get it to assemble with the force option: -
>>
>> "mdadm --assemble --force /dev/mdX <list of devices>"
>>
>> mdadm --assemble --force /dev/md0 /dev/sdd1 /dev/sdg1 /dev/sdf1
>/dev/sde1 /dev/sdb1 /dev/sdc1
>
>I don't think you need to explicitly list the devices (/dev/sd(..)) in
>the force
>
>You could do something like this:
>
>mdadm --examine --scan | grep md0 > /tmp/md0.conf
>mdadm -As -c /tmp/md0.conf
>
>if this doesn't work, then the force *may* be an option, but be very
>careful with this.
>>
Hi Joe
Thanks for the reply, unfortunately that came back as follows: -
[root@inscribe4 ~]# mdadm --examine --scan
ARRAY /dev/md/intermediate1 metadata=1.2 UUID=64a34aa3:3703e1b0:37d3173a:bec0c874 name=intermediate1
ARRAY /dev/md/intermediate0 metadata=1.2 UUID=ace00fb5:8771d55a:cf36728b:ddc3ce89 name=intermediate0
spares=1
ARRAY /dev/md/128 metadata=1.2 UUID=d59b4fac:ade4fdbf:13ebe850:5e70ba42 name=igmimager.ncl.ac.uk:128
[root@inscribe4 md]# mdadm --examine --scan | grep intermediate0 > /tmp/intermediate0.conf
[root@inscribe4 md]# cat /tmp/intermediate0.conf
ARRAY /dev/md/intermediate0 metadata=1.2 UUID=ace00fb5:8771d55a:cf36728b:ddc3ce89 name=intermediate0
[root@inscribe4 md]# mdadm --assemble --scan -c /tmp/intermediate0.conf
mdadm: /dev/md/intermediate0 assembled from 4 drives and 1 spare - not enough to start the array.
[root@inscribe4 md]#
Looks like I'll have to use the force option unless you can think of a better way
Thanks
Bryan
^ permalink raw reply
* Re: force raid reassembly
From: Joe Landman @ 2017-08-09 10:34 UTC (permalink / raw)
To: Bryan Hepworth, linux-raid
In-Reply-To: <WM!bafd27028d31e7e922c12b615def50baa8369ba598ac220b2884f1706eddac34cd0580e2d54cc3da9be1bbadf104dab1!@mailhub-mx1.ncl.ac.uk>
Ok, you are missing 2 drives in a 6 drive raid. This is generally bad
news. Are you sure all your drives are accessible? Can you read partition
tables from each? If not, f2f hen a force won't likely do anything.
Can you give context and logs entries on what happend to cause this? Also
do you have backups of the data... raid is not a backup itself ... if need
be, can you recover from a recent backup?
On August 8, 2017 3:39:46 PM Bryan Hepworth
<bryan.hepworth@newcastle.ac.uk> wrote:
> Hi all
>
> I'd be grateful of another pair of eyes just to check and verify what I'm
> thinking. I've had an array fail and I've checked the components with the
> mdadm --examine command. Five out of the six event counts are correct and
> I'm hoping I can get it to assemble with the force option: -
>
> "mdadm --assemble --force /dev/mdX <list of devices>"
>
> mdadm --assemble --force /dev/md0 /dev/sdd1 /dev/sdg1 /dev/sdf1 /dev/sde1
> /dev/sdb1 /dev/sdc1
>
> Output from --examine is as follows: -
>
>
> [root@inscribe4 dev]# mdadm --examine /dev/sde1
> /dev/sde1:
> Magic : a92b4efc
> Version : 1.2
> Feature Map : 0x1
> Array UUID : ace00fb5:8771d55a:cf36728b:ddc3ce89
> Name : intermediate0
> Creation Time : Tue Oct 15 21:03:50 2013
> Raid Level : raid5
> Raid Devices : 6
>
> Avail Dev Size : 3906761858 (1862.89 GiB 2000.26 GB)
> Array Size : 9766901760 (9314.44 GiB 10001.31 GB)
> Used Dev Size : 3906760704 (1862.89 GiB 2000.26 GB)
> Data Offset : 262144 sectors
> Super Offset : 8 sectors
> Unused Space : before=262064 sectors, after=1154 sectors
> State : clean
> Device UUID : 8d0b6c8e:0c526265:6ec0a71b:52a636ba
>
> Internal Bitmap : 8 sectors from superblock
> Update Time : Tue Aug 8 14:16:53 2017
> Checksum : 44b7b461 - correct
> Events : 119894
>
> Layout : left-symmetric
> Chunk Size : 512K
>
> Device Role : spare
> Array State : AAA.A. ('A' == active, '.' == missing, 'R' == replacing)
>
> [root@inscribe4 dev]# mdadm --examine /dev/sdg1
> /dev/sdg1:
> Magic : a92b4efc
> Version : 1.2
> Feature Map : 0x1
> Array UUID : ace00fb5:8771d55a:cf36728b:ddc3ce89
> Name : intermediate0
> Creation Time : Tue Oct 15 21:03:50 2013
> Raid Level : raid5
> Raid Devices : 6
>
> Avail Dev Size : 3906761858 (1862.89 GiB 2000.26 GB)
> Array Size : 9766901760 (9314.44 GiB 10001.31 GB)
> Used Dev Size : 3906760704 (1862.89 GiB 2000.26 GB)
> Data Offset : 262144 sectors
> Super Offset : 8 sectors
> Unused Space : before=262064 sectors, after=1154 sectors
> State : clean
> Device UUID : e238e74b:d40efc43:d34d876d:ca0700b0
>
> Internal Bitmap : 8 sectors from superblock
> Update Time : Tue Aug 8 14:16:53 2017
> Checksum : 28758d54 - correct
> Events : 119894
>
> Layout : left-symmetric
> Chunk Size : 512K
>
> Device Role : Active device 1
> Array State : AAA.A. ('A' == active, '.' == missing, 'R' == replacing)
>
> [root@inscribe4 dev]# mdadm --examine /dev/sdd1
> /dev/sdd1:
> Magic : a92b4efc
> Version : 1.2
> Feature Map : 0x1
> Array UUID : ace00fb5:8771d55a:cf36728b:ddc3ce89
> Name : intermediate0
> Creation Time : Tue Oct 15 21:03:50 2013
> Raid Level : raid5
> Raid Devices : 6
>
> Avail Dev Size : 3906761858 (1862.89 GiB 2000.26 GB)
> Array Size : 9766901760 (9314.44 GiB 10001.31 GB)
> Used Dev Size : 3906760704 (1862.89 GiB 2000.26 GB)
> Data Offset : 262144 sectors
> Super Offset : 8 sectors
> Unused Space : before=262064 sectors, after=1154 sectors
> State : clean
> Device UUID : 1732f121:f6f0df09:fafd2411:7c78ce00
>
> Internal Bitmap : 8 sectors from superblock
> Update Time : Tue Aug 8 14:16:53 2017
> Checksum : b8cf8981 - correct
> Events : 119894
>
> Layout : left-symmetric
> Chunk Size : 512K
>
> Device Role : Active device 0
> Array State : AAA.A. ('A' == active, '.' == missing, 'R' == replacing)
>
> [root@inscribe4 dev]# mdadm --examine /dev/sdf1
> /dev/sdf1:
> Magic : a92b4efc
> Version : 1.2
> Feature Map : 0x1
> Array UUID : ace00fb5:8771d55a:cf36728b:ddc3ce89
> Name : intermediate0
> Creation Time : Tue Oct 15 21:03:50 2013
> Raid Level : raid5
> Raid Devices : 6
>
> Avail Dev Size : 3906761858 (1862.89 GiB 2000.26 GB)
> Array Size : 9766901760 (9314.44 GiB 10001.31 GB)
> Used Dev Size : 3906760704 (1862.89 GiB 2000.26 GB)
> Data Offset : 262144 sectors
> Super Offset : 8 sectors
> Unused Space : before=262064 sectors, after=1154 sectors
> State : clean
> Device UUID : 3d414ad2:4310c1f9:98f50214:3e9b1e33
>
> Internal Bitmap : 8 sectors from superblock
> Update Time : Tue Aug 8 14:16:53 2017
> Checksum : 8e37d25d - correct
> Events : 119894
>
> Layout : left-symmetric
> Chunk Size : 512K
>
> Device Role : Active device 2
> Array State : AAA.A. ('A' == active, '.' == missing, 'R' == replacing)
>
> [root@inscribe4 dev]# mdadm --examine /dev/sdb1
> /dev/sdb1:
> Magic : a92b4efc
> Version : 1.2
> Feature Map : 0x1
> Array UUID : ace00fb5:8771d55a:cf36728b:ddc3ce89
> Name : intermediate0
> Creation Time : Tue Oct 15 21:03:50 2013
> Raid Level : raid5
> Raid Devices : 6
>
> Avail Dev Size : 3906761858 (1862.89 GiB 2000.26 GB)
> Array Size : 9766901760 (9314.44 GiB 10001.31 GB)
> Used Dev Size : 3906760704 (1862.89 GiB 2000.26 GB)
> Data Offset : 262144 sectors
> Super Offset : 8 sectors
> Unused Space : before=262064 sectors, after=1154 sectors
> State : clean
> Device UUID : a84263cd:75ce20b9:a3a72a1c:b18b2457
>
> Internal Bitmap : 8 sectors from superblock
> Update Time : Mon Jul 31 22:50:37 2017
> Checksum : 74d320d6 - correct
> Events : 119879
>
> Layout : left-symmetric
> Chunk Size : 512K
>
> Device Role : Active device 5
> Array State : AAA.AA ('A' == active, '.' == missing, 'R' == replacing)
>
> [root@inscribe4 dev]# mdadm --examine /dev/sdc1
> /dev/sdc1:
> Magic : a92b4efc
> Version : 1.2
> Feature Map : 0x1
> Array UUID : ace00fb5:8771d55a:cf36728b:ddc3ce89
> Name : intermediate0
> Creation Time : Tue Oct 15 21:03:50 2013
> Raid Level : raid5
> Raid Devices : 6
>
> Avail Dev Size : 3906761858 (1862.89 GiB 2000.26 GB)
> Array Size : 9766901760 (9314.44 GiB 10001.31 GB)
> Used Dev Size : 3906760704 (1862.89 GiB 2000.26 GB)
> Data Offset : 262144 sectors
> Super Offset : 8 sectors
> Unused Space : before=262064 sectors, after=1154 sectors
> State : clean
> Device UUID : be5ec20e:249645e5:22aeca25:a60c7599
>
> Internal Bitmap : 8 sectors from superblock
> Update Time : Tue Aug 8 14:16:53 2017
> Checksum : 2e529fae - correct
> Events : 119894
>
> Layout : left-symmetric
> Chunk Size : 512K
>
> Device Role : Active device 4
> Array State : AAA.A. ('A' == active, '.' == missing, 'R' == replacing)
>
> Thanks
>
> Bryan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/9] QUEUE_FLAG_NOWAIT to indicate device supports nowait
From: Goldwyn Rodrigues @ 2017-08-09 11:44 UTC (permalink / raw)
To: Shaohua Li
Cc: linux-block, hch, jack, linux-raid, dm-devel, Goldwyn Rodrigues
In-Reply-To: <20170808203246.sdjnpzixxz5brjaf@kernel.org>
On 08/08/2017 03:32 PM, Shaohua Li wrote:
> On Wed, Jul 26, 2017 at 06:57:58PM -0500, Goldwyn Rodrigues wrote:
>> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>
>> Nowait is a feature of direct AIO, where users can request
>> to return immediately if the I/O is going to block. This translates
>> to REQ_NOWAIT in bio.bi_opf flags. While request based devices
>> don't wait, stacked devices such as md/dm will.
>>
>> In order to explicitly mark stacked devices as supported, we
>> set the QUEUE_FLAG_NOWAIT in the queue_flags and return -EAGAIN
>> whenever the device would block.
>
> probably you should route this patch to Jens first, DM/MD are different trees.
Yes, I have sent it to linux-block as well, and he has commented as well.
>
>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
>> ---
>> block/blk-core.c | 3 ++-
>> include/linux/blkdev.h | 2 ++
>> 2 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index 970b9c9638c5..1c9a981d88e5 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -2025,7 +2025,8 @@ generic_make_request_checks(struct bio *bio)
>> * if queue is not a request based queue.
>> */
>>
>> - if ((bio->bi_opf & REQ_NOWAIT) && !queue_is_rq_based(q))
>> + if ((bio->bi_opf & REQ_NOWAIT) && !queue_is_rq_based(q) &&
>> + !blk_queue_supports_nowait(q))
>> goto not_supported;
>>
>> part = bio->bi_bdev->bd_part;
>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>> index 25f6a0cb27d3..fae021ebec1b 100644
>> --- a/include/linux/blkdev.h
>> +++ b/include/linux/blkdev.h
>> @@ -633,6 +633,7 @@ struct request_queue {
>> #define QUEUE_FLAG_REGISTERED 29 /* queue has been registered to a disk */
>> #define QUEUE_FLAG_SCSI_PASSTHROUGH 30 /* queue supports SCSI commands */
>> #define QUEUE_FLAG_QUIESCED 31 /* queue has been quiesced */
>> +#define QUEUE_FLAG_NOWAIT 32 /* stack device driver supports REQ_NOWAIT */
>>
>> #define QUEUE_FLAG_DEFAULT ((1 << QUEUE_FLAG_IO_STAT) | \
>> (1 << QUEUE_FLAG_STACKABLE) | \
>> @@ -732,6 +733,7 @@ static inline void queue_flag_clear(unsigned int flag, struct request_queue *q)
>> #define blk_queue_dax(q) test_bit(QUEUE_FLAG_DAX, &(q)->queue_flags)
>> #define blk_queue_scsi_passthrough(q) \
>> test_bit(QUEUE_FLAG_SCSI_PASSTHROUGH, &(q)->queue_flags)
>> +#define blk_queue_supports_nowait(q) test_bit(QUEUE_FLAG_NOWAIT, &(q)->queue_flags)
>
> Should this bit consider under layer disks? For example, one raid array disk
> doesn't support NOWAIT, shouldn't we disable NOWAIT for the array?
Yes, it should. I will add a check before setting the flag. Thanks.
Request-based devices don't wait. So, they would not have this flag set.
It is only the bio-based, with the make_request_fn hook which need this.
>
> I have another generic question. If a bio is splitted into 2 bios, one bio
> doesn't need to wait but the other need to wait. We will return -EAGAIN for the
> second bio, so the whole bio will return -EAGAIN, but the first bio is already
> dispatched to disk. Is this correct behavior?
>
No, from a multi-device point of view, this is inconsistent. I have
tried the request bio returns -EAGAIN before the split, but I shall
check again. Where do you see this happening?
--
Goldwyn
^ permalink raw reply
* Re: [PATCH 3/9] md: raid1 nowait support
From: Goldwyn Rodrigues @ 2017-08-09 11:45 UTC (permalink / raw)
To: Shaohua Li
Cc: linux-block, hch, jack, linux-raid, dm-devel, Goldwyn Rodrigues
In-Reply-To: <20170808203907.obfzlkqe4zboipva@kernel.org>
On 08/08/2017 03:39 PM, Shaohua Li wrote:
> On Wed, Jul 26, 2017 at 06:58:00PM -0500, Goldwyn Rodrigues wrote:
>> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>
>> The RAID1 driver would bail with EAGAIN in case of:
>> + I/O has to wait for a barrier
>> + array is frozen
>> + Area is suspended
>> + There are too many pending I/O that it will be queued.
>>
>> To facilitate error for wait barriers, wait_barrier() is
>> returning bool. True in case if there was a wait (or is not
>> required). False in case a wait was required, but was not performed.
>>
>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
>> ---
>> drivers/md/raid1.c | 74 +++++++++++++++++++++++++++++++++++++++++-------------
>> 1 file changed, 57 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
>> index 3febfc8391fb..66ca4288e3e8 100644
>> --- a/drivers/md/raid1.c
>> +++ b/drivers/md/raid1.c
>> @@ -903,8 +903,9 @@ static void lower_barrier(struct r1conf *conf, sector_t sector_nr)
>> wake_up(&conf->wait_barrier);
>> }
>>
>> -static void _wait_barrier(struct r1conf *conf, int idx)
>> +static bool _wait_barrier(struct r1conf *conf, int idx, bool nowait)
>> {
>> + bool ret = true;
>> /*
>> * We need to increase conf->nr_pending[idx] very early here,
>> * then raise_barrier() can be blocked when it waits for
>> @@ -935,7 +936,7 @@ static void _wait_barrier(struct r1conf *conf, int idx)
>> */
>> if (!READ_ONCE(conf->array_frozen) &&
>> !atomic_read(&conf->barrier[idx]))
>> - return;
>> + return ret;
>>
>> /*
>> * After holding conf->resync_lock, conf->nr_pending[idx]
>> @@ -953,18 +954,26 @@ static void _wait_barrier(struct r1conf *conf, int idx)
>> */
>> wake_up(&conf->wait_barrier);
>> /* Wait for the barrier in same barrier unit bucket to drop. */
>> - wait_event_lock_irq(conf->wait_barrier,
>> - !conf->array_frozen &&
>> - !atomic_read(&conf->barrier[idx]),
>> - conf->resync_lock);
>> + if (conf->array_frozen || atomic_read(&conf->barrier[idx])) {
>> + if (nowait)
>> + ret = false;
>
> In this case, we nr_pending shouldn't be increased
Ok, will fix this.
>
>> + else
>> + wait_event_lock_irq(conf->wait_barrier,
>> + !conf->array_frozen &&
>> + !atomic_read(&conf->barrier[idx]),
>> + conf->resync_lock);
>> + }
>> atomic_inc(&conf->nr_pending[idx]);
>> atomic_dec(&conf->nr_waiting[idx]);
>> spin_unlock_irq(&conf->resync_lock);
>> + return ret;
>> }
>>
>> -static void wait_read_barrier(struct r1conf *conf, sector_t sector_nr)
>> +static bool wait_read_barrier(struct r1conf *conf, sector_t sector_nr,
>> + bool nowait)
>> {
>> int idx = sector_to_idx(sector_nr);
>> + bool ret = true;
>>
>> /*
>> * Very similar to _wait_barrier(). The difference is, for read
>> @@ -976,7 +985,7 @@ static void wait_read_barrier(struct r1conf *conf, sector_t sector_nr)
>> atomic_inc(&conf->nr_pending[idx]);
>>
>> if (!READ_ONCE(conf->array_frozen))
>> - return;
>> + return ret;
>>
>> spin_lock_irq(&conf->resync_lock);
>> atomic_inc(&conf->nr_waiting[idx]);
>> @@ -987,19 +996,28 @@ static void wait_read_barrier(struct r1conf *conf, sector_t sector_nr)
>> */
>> wake_up(&conf->wait_barrier);
>> /* Wait for array to be unfrozen */
>> - wait_event_lock_irq(conf->wait_barrier,
>> - !conf->array_frozen,
>> - conf->resync_lock);
>> + if (conf->array_frozen) {
>> + /* If nowait flag is set, return false to
>> + * show we did not wait
>> + */
>> + if (nowait)
>> + ret = false;
>
> ditto
>> + else
>> + wait_event_lock_irq(conf->wait_barrier,
>> + !conf->array_frozen,
>> + conf->resync_lock);
>> + }
>> atomic_inc(&conf->nr_pending[idx]);
>> atomic_dec(&conf->nr_waiting[idx]);
>> spin_unlock_irq(&conf->resync_lock);
>> + return ret;
>> }
>>
>> -static void wait_barrier(struct r1conf *conf, sector_t sector_nr)
>> +static bool wait_barrier(struct r1conf *conf, sector_t sector_nr, bool nowait)
>> {
>> int idx = sector_to_idx(sector_nr);
>>
>> - _wait_barrier(conf, idx);
>> + return _wait_barrier(conf, idx, nowait);
>> }
>>
>> static void wait_all_barriers(struct r1conf *conf)
>> @@ -1007,7 +1025,7 @@ static void wait_all_barriers(struct r1conf *conf)
>> int idx;
>>
>> for (idx = 0; idx < BARRIER_BUCKETS_NR; idx++)
>> - _wait_barrier(conf, idx);
>> + _wait_barrier(conf, idx, false);
>> }
>>
>> static void _allow_barrier(struct r1conf *conf, int idx)
>> @@ -1223,7 +1241,11 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
>> * Still need barrier for READ in case that whole
>> * array is frozen.
>> */
>> - wait_read_barrier(conf, bio->bi_iter.bi_sector);
>> + if (!wait_read_barrier(conf, bio->bi_iter.bi_sector,
>> + bio->bi_opf & REQ_NOWAIT)) {
>> + bio_wouldblock_error(bio);
>> + return;
>> + }
>>
>> if (!r1_bio)
>> r1_bio = alloc_r1bio(mddev, bio);
>> @@ -1333,6 +1355,11 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
>> * an interruptible wait.
>> */
>> DEFINE_WAIT(w);
>> + if (bio->bi_opf & REQ_NOWAIT) {
>> + bio_wouldblock_error(bio);
>> + return;
>> + }
>> +
>> for (;;) {
>> sigset_t full, old;
>> prepare_to_wait(&conf->wait_barrier,
>> @@ -1351,7 +1378,11 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
>> }
>> finish_wait(&conf->wait_barrier, &w);
>> }
>> - wait_barrier(conf, bio->bi_iter.bi_sector);
>> + if (!wait_barrier(conf, bio->bi_iter.bi_sector,
>> + bio->bi_opf & REQ_NOWAIT)) {
>> + bio_wouldblock_error(bio);
>> + return;
>> + }
>>
>> r1_bio = alloc_r1bio(mddev, bio);
>> r1_bio->sectors = max_write_sectors;
>> @@ -1359,6 +1390,10 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
>> if (conf->pending_count >= max_queued_requests) {
>> md_wakeup_thread(mddev->thread);
>> raid1_log(mddev, "wait queued");
>> + if (bio->bi_opf & REQ_NOWAIT) {
>> + bio_wouldblock_error(bio);
>> + return;
>> + }
>> wait_event(conf->wait_barrier,
>> conf->pending_count < max_queued_requests);
>> }
>> @@ -1442,6 +1477,11 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
>> /* Wait for this device to become unblocked */
>> int j;
>>
>> + if (bio->bi_opf & REQ_NOWAIT) {
>> + bio_wouldblock_error(bio);
>> + return;
>> + }
>> +
>> for (j = 0; j < i; j++)
>> if (r1_bio->bios[j])
>> rdev_dec_pending(conf->mirrors[j].rdev, mddev);
>> @@ -1449,7 +1489,7 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
>> allow_barrier(conf, bio->bi_iter.bi_sector);
>> raid1_log(mddev, "wait rdev %d blocked", blocked_rdev->raid_disk);
>> md_wait_for_blocked_rdev(blocked_rdev, mddev);
>> - wait_barrier(conf, bio->bi_iter.bi_sector);
>> + wait_barrier(conf, bio->bi_iter.bi_sector, false);
>
> There are other cases we could block, for example, md_wait_for_blocked_rdev
> here. Is the goal just avoid block for normal situations?
Isn't this covered by the if condition of the codeblock (blocked_rdev !=
NULL)?
>
>> goto retry_write;
>> }
>>
>> --
>> 2.12.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Goldwyn
^ permalink raw reply
* Re: [PATCH 4/9] md: raid5 nowait support
From: Goldwyn Rodrigues @ 2017-08-09 11:45 UTC (permalink / raw)
To: Shaohua Li
Cc: linux-block, hch, jack, linux-raid, dm-devel, Goldwyn Rodrigues
In-Reply-To: <20170808204343.mzt5ujazevrkyro3@kernel.org>
On 08/08/2017 03:43 PM, Shaohua Li wrote:
> On Wed, Jul 26, 2017 at 06:58:01PM -0500, Goldwyn Rodrigues wrote:
>> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>
>> Return EAGAIN in case RAID5 would block because of waiting due to:
>> + Reshaping
>> + Suspension
>> + Stripe Expansion
>>
>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
>> ---
>> drivers/md/raid5.c | 20 ++++++++++++++++++++
>> 1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>> index aeeb8d6854e2..d1b3bcf26d29 100644
>> --- a/drivers/md/raid5.c
>> +++ b/drivers/md/raid5.c
>> @@ -5635,6 +5635,11 @@ static bool raid5_make_request(struct mddev *mddev, struct bio * bi)
>> ? logical_sector < conf->reshape_safe
>> : logical_sector >= conf->reshape_safe) {
>> spin_unlock_irq(&conf->device_lock);
>> + if (bi->bi_opf & REQ_NOWAIT) {
>> + bio_wouldblock_error(bi);
>> + finish_wait(&conf->wait_for_overlap, &w);
>> + return true;
>> + }
>
> A bio could use several stripes. If one stripe block, simpliy return bio here
> doesn't really make the whole bio finish.
My understanding is a little weak here.
How would you have to terminate the entire bio since one of the stripes
is blocking?
>
>> schedule();
>> do_prepare = true;
>> goto retry;
>> @@ -5672,6 +5677,11 @@ static bool raid5_make_request(struct mddev *mddev, struct bio * bi)
>> spin_unlock_irq(&conf->device_lock);
>> if (must_retry) {
>> raid5_release_stripe(sh);
>> + if (bi->bi_opf & REQ_NOWAIT) {
>> + bio_wouldblock_error(bi);
>> + finish_wait(&conf->wait_for_overlap, &w);
>> + return true;
>> + }
>> schedule();
>> do_prepare = true;
>> goto retry;
>> @@ -5700,6 +5710,11 @@ static bool raid5_make_request(struct mddev *mddev, struct bio * bi)
>> sigset_t full, old;
>> sigfillset(&full);
>> sigprocmask(SIG_BLOCK, &full, &old);
>> + if (bi->bi_opf & REQ_NOWAIT) {
>> + bio_wouldblock_error(bi);
>> + finish_wait(&conf->wait_for_overlap, &w);
>> + return true;
>> + }
>> schedule();
>> sigprocmask(SIG_SETMASK, &old, NULL);
>> do_prepare = true;
>> @@ -5715,6 +5730,11 @@ static bool raid5_make_request(struct mddev *mddev, struct bio * bi)
>> */
>> md_wakeup_thread(mddev->thread);
>> raid5_release_stripe(sh);
>> + if (bi->bi_opf & REQ_NOWAIT) {
>> + bio_wouldblock_error(bi);
>> + finish_wait(&conf->wait_for_overlap, &w);
>> + return true;
>> + }
>> schedule();
>> do_prepare = true;
>> goto retry;
>> --
>> 2.12.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Goldwyn
^ permalink raw reply
* Re: [PATCH 1/9] QUEUE_FLAG_NOWAIT to indicate device supports nowait
From: Shaohua Li @ 2017-08-09 15:02 UTC (permalink / raw)
To: Goldwyn Rodrigues
Cc: linux-block, hch, jack, linux-raid, dm-devel, Goldwyn Rodrigues
In-Reply-To: <51cdf08c-c8a4-6bbd-3342-7cdb2b510668@suse.de>
On Wed, Aug 09, 2017 at 06:44:55AM -0500, Goldwyn Rodrigues wrote:
>
>
> On 08/08/2017 03:32 PM, Shaohua Li wrote:
> > On Wed, Jul 26, 2017 at 06:57:58PM -0500, Goldwyn Rodrigues wrote:
> >> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> >>
> >> Nowait is a feature of direct AIO, where users can request
> >> to return immediately if the I/O is going to block. This translates
> >> to REQ_NOWAIT in bio.bi_opf flags. While request based devices
> >> don't wait, stacked devices such as md/dm will.
> >>
> >> In order to explicitly mark stacked devices as supported, we
> >> set the QUEUE_FLAG_NOWAIT in the queue_flags and return -EAGAIN
> >> whenever the device would block.
> >
> > probably you should route this patch to Jens first, DM/MD are different trees.
>
> Yes, I have sent it to linux-block as well, and he has commented as well.
>
>
> >
> >> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> >> ---
> >> block/blk-core.c | 3 ++-
> >> include/linux/blkdev.h | 2 ++
> >> 2 files changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/block/blk-core.c b/block/blk-core.c
> >> index 970b9c9638c5..1c9a981d88e5 100644
> >> --- a/block/blk-core.c
> >> +++ b/block/blk-core.c
> >> @@ -2025,7 +2025,8 @@ generic_make_request_checks(struct bio *bio)
> >> * if queue is not a request based queue.
> >> */
> >>
> >> - if ((bio->bi_opf & REQ_NOWAIT) && !queue_is_rq_based(q))
> >> + if ((bio->bi_opf & REQ_NOWAIT) && !queue_is_rq_based(q) &&
> >> + !blk_queue_supports_nowait(q))
> >> goto not_supported;
> >>
> >> part = bio->bi_bdev->bd_part;
> >> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> >> index 25f6a0cb27d3..fae021ebec1b 100644
> >> --- a/include/linux/blkdev.h
> >> +++ b/include/linux/blkdev.h
> >> @@ -633,6 +633,7 @@ struct request_queue {
> >> #define QUEUE_FLAG_REGISTERED 29 /* queue has been registered to a disk */
> >> #define QUEUE_FLAG_SCSI_PASSTHROUGH 30 /* queue supports SCSI commands */
> >> #define QUEUE_FLAG_QUIESCED 31 /* queue has been quiesced */
> >> +#define QUEUE_FLAG_NOWAIT 32 /* stack device driver supports REQ_NOWAIT */
> >>
> >> #define QUEUE_FLAG_DEFAULT ((1 << QUEUE_FLAG_IO_STAT) | \
> >> (1 << QUEUE_FLAG_STACKABLE) | \
> >> @@ -732,6 +733,7 @@ static inline void queue_flag_clear(unsigned int flag, struct request_queue *q)
> >> #define blk_queue_dax(q) test_bit(QUEUE_FLAG_DAX, &(q)->queue_flags)
> >> #define blk_queue_scsi_passthrough(q) \
> >> test_bit(QUEUE_FLAG_SCSI_PASSTHROUGH, &(q)->queue_flags)
> >> +#define blk_queue_supports_nowait(q) test_bit(QUEUE_FLAG_NOWAIT, &(q)->queue_flags)
> >
> > Should this bit consider under layer disks? For example, one raid array disk
> > doesn't support NOWAIT, shouldn't we disable NOWAIT for the array?
>
> Yes, it should. I will add a check before setting the flag. Thanks.
> Request-based devices don't wait. So, they would not have this flag set.
> It is only the bio-based, with the make_request_fn hook which need this.
>
> >
> > I have another generic question. If a bio is splitted into 2 bios, one bio
> > doesn't need to wait but the other need to wait. We will return -EAGAIN for the
> > second bio, so the whole bio will return -EAGAIN, but the first bio is already
> > dispatched to disk. Is this correct behavior?
> >
>
> No, from a multi-device point of view, this is inconsistent. I have
> tried the request bio returns -EAGAIN before the split, but I shall
> check again. Where do you see this happening?
No, this isn't multi-device specific, any driver can do it. Please see blk_queue_split.
^ permalink raw reply
* Re: [PATCH 1/9] QUEUE_FLAG_NOWAIT to indicate device supports nowait
From: Goldwyn Rodrigues @ 2017-08-09 15:35 UTC (permalink / raw)
To: Shaohua Li
Cc: linux-block, hch, jack, linux-raid, dm-devel, Goldwyn Rodrigues
In-Reply-To: <20170809150205.4dm3vskyjqzyejsq@kernel.org>
On 08/09/2017 10:02 AM, Shaohua Li wrote:
> On Wed, Aug 09, 2017 at 06:44:55AM -0500, Goldwyn Rodrigues wrote:
>>
>>
>> On 08/08/2017 03:32 PM, Shaohua Li wrote:
>>> On Wed, Jul 26, 2017 at 06:57:58PM -0500, Goldwyn Rodrigues wrote:
>>>> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>>>
>>>> Nowait is a feature of direct AIO, where users can request
>>>> to return immediately if the I/O is going to block. This translates
>>>> to REQ_NOWAIT in bio.bi_opf flags. While request based devices
>>>> don't wait, stacked devices such as md/dm will.
>>>>
>>>> In order to explicitly mark stacked devices as supported, we
>>>> set the QUEUE_FLAG_NOWAIT in the queue_flags and return -EAGAIN
>>>> whenever the device would block.
>>>
>>> probably you should route this patch to Jens first, DM/MD are different trees.
>>
>> Yes, I have sent it to linux-block as well, and he has commented as well.
>>
>>
>>>
>>>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>>> ---
>>>> block/blk-core.c | 3 ++-
>>>> include/linux/blkdev.h | 2 ++
>>>> 2 files changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>>> index 970b9c9638c5..1c9a981d88e5 100644
>>>> --- a/block/blk-core.c
>>>> +++ b/block/blk-core.c
>>>> @@ -2025,7 +2025,8 @@ generic_make_request_checks(struct bio *bio)
>>>> * if queue is not a request based queue.
>>>> */
>>>>
>>>> - if ((bio->bi_opf & REQ_NOWAIT) && !queue_is_rq_based(q))
>>>> + if ((bio->bi_opf & REQ_NOWAIT) && !queue_is_rq_based(q) &&
>>>> + !blk_queue_supports_nowait(q))
>>>> goto not_supported;
>>>>
>>>> part = bio->bi_bdev->bd_part;
>>>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>>>> index 25f6a0cb27d3..fae021ebec1b 100644
>>>> --- a/include/linux/blkdev.h
>>>> +++ b/include/linux/blkdev.h
>>>> @@ -633,6 +633,7 @@ struct request_queue {
>>>> #define QUEUE_FLAG_REGISTERED 29 /* queue has been registered to a disk */
>>>> #define QUEUE_FLAG_SCSI_PASSTHROUGH 30 /* queue supports SCSI commands */
>>>> #define QUEUE_FLAG_QUIESCED 31 /* queue has been quiesced */
>>>> +#define QUEUE_FLAG_NOWAIT 32 /* stack device driver supports REQ_NOWAIT */
>>>>
>>>> #define QUEUE_FLAG_DEFAULT ((1 << QUEUE_FLAG_IO_STAT) | \
>>>> (1 << QUEUE_FLAG_STACKABLE) | \
>>>> @@ -732,6 +733,7 @@ static inline void queue_flag_clear(unsigned int flag, struct request_queue *q)
>>>> #define blk_queue_dax(q) test_bit(QUEUE_FLAG_DAX, &(q)->queue_flags)
>>>> #define blk_queue_scsi_passthrough(q) \
>>>> test_bit(QUEUE_FLAG_SCSI_PASSTHROUGH, &(q)->queue_flags)
>>>> +#define blk_queue_supports_nowait(q) test_bit(QUEUE_FLAG_NOWAIT, &(q)->queue_flags)
>>>
>>> Should this bit consider under layer disks? For example, one raid array disk
>>> doesn't support NOWAIT, shouldn't we disable NOWAIT for the array?
>>
>> Yes, it should. I will add a check before setting the flag. Thanks.
>> Request-based devices don't wait. So, they would not have this flag set.
>> It is only the bio-based, with the make_request_fn hook which need this.
>>
>>>
>>> I have another generic question. If a bio is splitted into 2 bios, one bio
>>> doesn't need to wait but the other need to wait. We will return -EAGAIN for the
>>> second bio, so the whole bio will return -EAGAIN, but the first bio is already
>>> dispatched to disk. Is this correct behavior?
>>>
>>
>> No, from a multi-device point of view, this is inconsistent. I have
>> tried the request bio returns -EAGAIN before the split, but I shall
>> check again. Where do you see this happening?
>
> No, this isn't multi-device specific, any driver can do it. Please see blk_queue_split.
>
In that case, the bio end_io function is chained and the bio of the
split will replicate the error to the parent (if not already set).
--
Goldwyn
^ permalink raw reply
* Re: [PATCH 0/2] md/raid6: improvements for ARM/arm64
From: Catalin Marinas @ 2017-08-09 17:51 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-arm-kernel, linux-raid, will.deacon, linux, stockhausen,
shli
In-Reply-To: <20170713171601.25440-1-ard.biesheuvel@linaro.org>
Hi Ard,
On Thu, Jul 13, 2017 at 06:15:59PM +0100, Ard Biesheuvel wrote:
> 1. Use a faster algorithm for the delta syndrome
> 2. Implement recovery routines in NEON
>
> As before, NEON intrinsics are used, which means the same code can be
> compiled for ARM as well as arm64.
>
> Given that there does not seem to be a maintainer for lib/raid6, could
> we take this through one of the ARM trees instead?
>
> Ard Biesheuvel (2):
> md/raid6: use faster multiplication for ARM NEON delta syndrome
> md/raid6: implement recovery using ARM NEON intrinsics
>
> include/linux/raid/pq.h | 1 +
> lib/raid6/Makefile | 4 +-
> lib/raid6/algos.c | 3 +
> lib/raid6/neon.uc | 33 +++++-
> lib/raid6/recov_neon.c | 110 ++++++++++++++++++
> lib/raid6/recov_neon_inner.c | 117 ++++++++++++++++++++
IIRC, you wanted these patches merged via the arm64 tree? I'll apply
them to the for-next/core branch.
--
Catalin
^ permalink raw reply
* Re: [PATCH 0/2] md/raid6: improvements for ARM/arm64
From: Ard Biesheuvel @ 2017-08-09 17:52 UTC (permalink / raw)
To: Catalin Marinas
Cc: linux-arm-kernel@lists.infradead.org, linux-raid, Will Deacon,
Russell King, Markus Stockhausen, shli
In-Reply-To: <20170809175137.hpj27jp2f4nen45g@armageddon.cambridge.arm.com>
On 9 August 2017 at 18:51, Catalin Marinas <catalin.marinas@arm.com> wrote:
> Hi Ard,
>
> On Thu, Jul 13, 2017 at 06:15:59PM +0100, Ard Biesheuvel wrote:
>> 1. Use a faster algorithm for the delta syndrome
>> 2. Implement recovery routines in NEON
>>
>> As before, NEON intrinsics are used, which means the same code can be
>> compiled for ARM as well as arm64.
>>
>> Given that there does not seem to be a maintainer for lib/raid6, could
>> we take this through one of the ARM trees instead?
>>
>> Ard Biesheuvel (2):
>> md/raid6: use faster multiplication for ARM NEON delta syndrome
>> md/raid6: implement recovery using ARM NEON intrinsics
>>
>> include/linux/raid/pq.h | 1 +
>> lib/raid6/Makefile | 4 +-
>> lib/raid6/algos.c | 3 +
>> lib/raid6/neon.uc | 33 +++++-
>> lib/raid6/recov_neon.c | 110 ++++++++++++++++++
>> lib/raid6/recov_neon_inner.c | 117 ++++++++++++++++++++
>
> IIRC, you wanted these patches merged via the arm64 tree? I'll apply
> them to the for-next/core branch.
>
Yes, please.
^ permalink raw reply
* Re: [PATCH 1/9] QUEUE_FLAG_NOWAIT to indicate device supports nowait
From: Shaohua Li @ 2017-08-09 20:21 UTC (permalink / raw)
To: Goldwyn Rodrigues
Cc: linux-block, hch, jack, linux-raid, dm-devel, Goldwyn Rodrigues
In-Reply-To: <de5000a0-a52c-bce4-de4f-ab39aae4b593@suse.de>
On Wed, Aug 09, 2017 at 10:35:39AM -0500, Goldwyn Rodrigues wrote:
>
>
> On 08/09/2017 10:02 AM, Shaohua Li wrote:
> > On Wed, Aug 09, 2017 at 06:44:55AM -0500, Goldwyn Rodrigues wrote:
> >>
> >>
> >> On 08/08/2017 03:32 PM, Shaohua Li wrote:
> >>> On Wed, Jul 26, 2017 at 06:57:58PM -0500, Goldwyn Rodrigues wrote:
> >>>> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> >>>>
> >>>> Nowait is a feature of direct AIO, where users can request
> >>>> to return immediately if the I/O is going to block. This translates
> >>>> to REQ_NOWAIT in bio.bi_opf flags. While request based devices
> >>>> don't wait, stacked devices such as md/dm will.
> >>>>
> >>>> In order to explicitly mark stacked devices as supported, we
> >>>> set the QUEUE_FLAG_NOWAIT in the queue_flags and return -EAGAIN
> >>>> whenever the device would block.
> >>>
> >>> probably you should route this patch to Jens first, DM/MD are different trees.
> >>
> >> Yes, I have sent it to linux-block as well, and he has commented as well.
> >>
> >>
> >>>
> >>>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> >>>> ---
> >>>> block/blk-core.c | 3 ++-
> >>>> include/linux/blkdev.h | 2 ++
> >>>> 2 files changed, 4 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/block/blk-core.c b/block/blk-core.c
> >>>> index 970b9c9638c5..1c9a981d88e5 100644
> >>>> --- a/block/blk-core.c
> >>>> +++ b/block/blk-core.c
> >>>> @@ -2025,7 +2025,8 @@ generic_make_request_checks(struct bio *bio)
> >>>> * if queue is not a request based queue.
> >>>> */
> >>>>
> >>>> - if ((bio->bi_opf & REQ_NOWAIT) && !queue_is_rq_based(q))
> >>>> + if ((bio->bi_opf & REQ_NOWAIT) && !queue_is_rq_based(q) &&
> >>>> + !blk_queue_supports_nowait(q))
> >>>> goto not_supported;
> >>>>
> >>>> part = bio->bi_bdev->bd_part;
> >>>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> >>>> index 25f6a0cb27d3..fae021ebec1b 100644
> >>>> --- a/include/linux/blkdev.h
> >>>> +++ b/include/linux/blkdev.h
> >>>> @@ -633,6 +633,7 @@ struct request_queue {
> >>>> #define QUEUE_FLAG_REGISTERED 29 /* queue has been registered to a disk */
> >>>> #define QUEUE_FLAG_SCSI_PASSTHROUGH 30 /* queue supports SCSI commands */
> >>>> #define QUEUE_FLAG_QUIESCED 31 /* queue has been quiesced */
> >>>> +#define QUEUE_FLAG_NOWAIT 32 /* stack device driver supports REQ_NOWAIT */
> >>>>
> >>>> #define QUEUE_FLAG_DEFAULT ((1 << QUEUE_FLAG_IO_STAT) | \
> >>>> (1 << QUEUE_FLAG_STACKABLE) | \
> >>>> @@ -732,6 +733,7 @@ static inline void queue_flag_clear(unsigned int flag, struct request_queue *q)
> >>>> #define blk_queue_dax(q) test_bit(QUEUE_FLAG_DAX, &(q)->queue_flags)
> >>>> #define blk_queue_scsi_passthrough(q) \
> >>>> test_bit(QUEUE_FLAG_SCSI_PASSTHROUGH, &(q)->queue_flags)
> >>>> +#define blk_queue_supports_nowait(q) test_bit(QUEUE_FLAG_NOWAIT, &(q)->queue_flags)
> >>>
> >>> Should this bit consider under layer disks? For example, one raid array disk
> >>> doesn't support NOWAIT, shouldn't we disable NOWAIT for the array?
> >>
> >> Yes, it should. I will add a check before setting the flag. Thanks.
> >> Request-based devices don't wait. So, they would not have this flag set.
> >> It is only the bio-based, with the make_request_fn hook which need this.
> >>
> >>>
> >>> I have another generic question. If a bio is splitted into 2 bios, one bio
> >>> doesn't need to wait but the other need to wait. We will return -EAGAIN for the
> >>> second bio, so the whole bio will return -EAGAIN, but the first bio is already
> >>> dispatched to disk. Is this correct behavior?
> >>>
> >>
> >> No, from a multi-device point of view, this is inconsistent. I have
> >> tried the request bio returns -EAGAIN before the split, but I shall
> >> check again. Where do you see this happening?
> >
> > No, this isn't multi-device specific, any driver can do it. Please see blk_queue_split.
> >
>
> In that case, the bio end_io function is chained and the bio of the
> split will replicate the error to the parent (if not already set).
this doesn't answer my question. So if a bio returns -EAGAIN, part of the bio
probably already dispatched to disk (if the bio is splitted to 2 bios, one
returns -EAGAIN, the other one doesn't block and dispatch to disk), what will
application be going to do? I think this is different to other IO errors. FOr
other IO errors, application will handle the error, while we ask app to retry
the whole bio here and app doesn't know part of bio is already written to disk.
^ permalink raw reply
* Re: [PATCH 1/9] QUEUE_FLAG_NOWAIT to indicate device supports nowait
From: Goldwyn Rodrigues @ 2017-08-09 22:16 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-block, hch, jack, linux-raid, dm-devel
In-Reply-To: <20170809202125.5zs4h45luqlub5if@kernel.org>
On 08/09/2017 03:21 PM, Shaohua Li wrote:
> On Wed, Aug 09, 2017 at 10:35:39AM -0500, Goldwyn Rodrigues wrote:
>>
>>
>> On 08/09/2017 10:02 AM, Shaohua Li wrote:
>>> On Wed, Aug 09, 2017 at 06:44:55AM -0500, Goldwyn Rodrigues wrote:
>>>>
>>>>
>>>> On 08/08/2017 03:32 PM, Shaohua Li wrote:
>>>>> On Wed, Jul 26, 2017 at 06:57:58PM -0500, Goldwyn Rodrigues wrote:
>>>>>> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>>>>>
>>>>>> Nowait is a feature of direct AIO, where users can request
>>>>>> to return immediately if the I/O is going to block. This translates
>>>>>> to REQ_NOWAIT in bio.bi_opf flags. While request based devices
>>>>>> don't wait, stacked devices such as md/dm will.
>>>>>>
>>>>>> In order to explicitly mark stacked devices as supported, we
>>>>>> set the QUEUE_FLAG_NOWAIT in the queue_flags and return -EAGAIN
>>>>>> whenever the device would block.
>>>>>
>>>>> probably you should route this patch to Jens first, DM/MD are different trees.
>>>>
>>>> Yes, I have sent it to linux-block as well, and he has commented as well.
>>>>
>>>>
>>>>>
>>>>>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>>>>> ---
>>>>>> block/blk-core.c | 3 ++-
>>>>>> include/linux/blkdev.h | 2 ++
>>>>>> 2 files changed, 4 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>>>>> index 970b9c9638c5..1c9a981d88e5 100644
>>>>>> --- a/block/blk-core.c
>>>>>> +++ b/block/blk-core.c
>>>>>> @@ -2025,7 +2025,8 @@ generic_make_request_checks(struct bio *bio)
>>>>>> * if queue is not a request based queue.
>>>>>> */
>>>>>>
>>>>>> - if ((bio->bi_opf & REQ_NOWAIT) && !queue_is_rq_based(q))
>>>>>> + if ((bio->bi_opf & REQ_NOWAIT) && !queue_is_rq_based(q) &&
>>>>>> + !blk_queue_supports_nowait(q))
>>>>>> goto not_supported;
>>>>>>
>>>>>> part = bio->bi_bdev->bd_part;
>>>>>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>>>>>> index 25f6a0cb27d3..fae021ebec1b 100644
>>>>>> --- a/include/linux/blkdev.h
>>>>>> +++ b/include/linux/blkdev.h
>>>>>> @@ -633,6 +633,7 @@ struct request_queue {
>>>>>> #define QUEUE_FLAG_REGISTERED 29 /* queue has been registered to a disk */
>>>>>> #define QUEUE_FLAG_SCSI_PASSTHROUGH 30 /* queue supports SCSI commands */
>>>>>> #define QUEUE_FLAG_QUIESCED 31 /* queue has been quiesced */
>>>>>> +#define QUEUE_FLAG_NOWAIT 32 /* stack device driver supports REQ_NOWAIT */
>>>>>>
>>>>>> #define QUEUE_FLAG_DEFAULT ((1 << QUEUE_FLAG_IO_STAT) | \
>>>>>> (1 << QUEUE_FLAG_STACKABLE) | \
>>>>>> @@ -732,6 +733,7 @@ static inline void queue_flag_clear(unsigned int flag, struct request_queue *q)
>>>>>> #define blk_queue_dax(q) test_bit(QUEUE_FLAG_DAX, &(q)->queue_flags)
>>>>>> #define blk_queue_scsi_passthrough(q) \
>>>>>> test_bit(QUEUE_FLAG_SCSI_PASSTHROUGH, &(q)->queue_flags)
>>>>>> +#define blk_queue_supports_nowait(q) test_bit(QUEUE_FLAG_NOWAIT, &(q)->queue_flags)
>>>>>
>>>>> Should this bit consider under layer disks? For example, one raid array disk
>>>>> doesn't support NOWAIT, shouldn't we disable NOWAIT for the array?
>>>>
>>>> Yes, it should. I will add a check before setting the flag. Thanks.
>>>> Request-based devices don't wait. So, they would not have this flag set.
>>>> It is only the bio-based, with the make_request_fn hook which need this.
>>>>
>>>>>
>>>>> I have another generic question. If a bio is splitted into 2 bios, one bio
>>>>> doesn't need to wait but the other need to wait. We will return -EAGAIN for the
>>>>> second bio, so the whole bio will return -EAGAIN, but the first bio is already
>>>>> dispatched to disk. Is this correct behavior?
>>>>>
>>>>
>>>> No, from a multi-device point of view, this is inconsistent. I have
>>>> tried the request bio returns -EAGAIN before the split, but I shall
>>>> check again. Where do you see this happening?
>>>
>>> No, this isn't multi-device specific, any driver can do it. Please see blk_queue_split.
>>>
>>
>> In that case, the bio end_io function is chained and the bio of the
>> split will replicate the error to the parent (if not already set).
>
> this doesn't answer my question. So if a bio returns -EAGAIN, part of the bio
> probably already dispatched to disk (if the bio is splitted to 2 bios, one
> returns -EAGAIN, the other one doesn't block and dispatch to disk), what will
> application be going to do? I think this is different to other IO errors. FOr
> other IO errors, application will handle the error, while we ask app to retry
> the whole bio here and app doesn't know part of bio is already written to disk.
It is the same as for other I/O errors as well, such as EIO. You do not
know which bio of all submitted bio's returned the error EIO. The
application would and should consider the whole I/O as failed.
The user application does not know of bios, or how it is going to be
split in the underlying layers. It knows at the system call level. In
this case, the EAGAIN will be returned to the user for the whole I/O not
as a part of the I/O. It is up to application to try the I/O again with
or without RWF_NOWAIT set. In direct I/O, it is bubbled out using
dio->io_error. You can read about it at the patch header for the initial
patchset at [1].
Use case: It is for applications having two threads, a compute thread
and an I/O thread. It would try to push AIO as much as possible in the
compute thread using RWF_NOWAIT, and if it fails, would pass it on to
I/O thread which would perform without RWF_NOWAIT. End result if done
right is you save on context switches and all the
synchronization/messaging machinery to perform I/O.
[1] http://marc.info/?l=linux-block&m=149789003305876&w=2
--
Goldwyn
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox