* Re: [PATCH 5/6] Check write journal in incremental
From: Dan Williams @ 2015-10-06 20:17 UTC (permalink / raw)
To: Song Liu; +Cc: linux-raid, Shaohua Li, Neil Brown, Christoph Hellwig
In-Reply-To: <1440804426-1461372-6-git-send-email-songliubraving@fb.com>
On Fri, Aug 28, 2015 at 4:27 PM, Song Liu <songliubraving@fb.com> wrote:
> If journal device is missing, do not start the array, and shows:
>
> ./mdadm -I /dev/sdf
> mdadm: journal device is missing, not safe to start yet.
>
> The array will be started when the journal device is attached with -I
>
> ./mdadm -I /dev/sdb1
> mdadm: /dev/sdb1 attached to /dev/md/0_0, which has been started.
>
> To force start without journal device:
>
> ./mdadm -I /dev/sdf --run
> mdadm: Trying to run with missing journal device
> mdadm: /dev/sdf attached to /dev/md/0_0, which has been started.
>
> Signed-off-by: Shaohua Li <shli@fb.com>
> Signed-off-by: Song Liu <songliubraving@fb.com>
> ---
> Incremental.c | 31 +++++++++++++++++++++++++++----
> 1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/Incremental.c b/Incremental.c
> index 304cc6d..74905e3 100644
> --- a/Incremental.c
> +++ b/Incremental.c
> @@ -35,7 +35,7 @@
>
> static int count_active(struct supertype *st, struct mdinfo *sra,
> int mdfd, char **availp,
> - struct mdinfo *info);
> + struct mdinfo *info, int *journal_device_missing);
> static void find_reject(int mdfd, struct supertype *st, struct mdinfo *sra,
> int number, __u64 events, int verbose,
> char *array_name);
> @@ -104,6 +104,7 @@ int Incremental(struct mddev_dev *devlist, struct context *c,
> struct map_ent target_array;
> int have_target;
> char *devname = devlist->devname;
> + int journal_device_missing = 0;
>
> struct createinfo *ci = conf_get_create_info();
>
> @@ -518,7 +519,7 @@ int Incremental(struct mddev_dev *devlist, struct context *c,
> sysfs_free(sra);
> sra = sysfs_read(mdfd, NULL, (GET_DEVS | GET_STATE |
> GET_OFFSET | GET_SIZE));
> - active_disks = count_active(st, sra, mdfd, &avail, &info);
> + active_disks = count_active(st, sra, mdfd, &avail, &info, &journal_device_missing);
> if (enough(info.array.level, info.array.raid_disks,
> info.array.layout, info.array.state & 1,
> avail) == 0) {
> @@ -548,10 +549,12 @@ int Incremental(struct mddev_dev *devlist, struct context *c,
> }
>
> map_unlock(&map);
> - if (c->runstop > 0 || active_disks >= info.array.working_disks) {
> + if (c->runstop > 0 || (!journal_device_missing && active_disks >= info.array.working_disks)) {
A minor comment, and I'd defer to Neil's opinion, but I think this is
asking for mdu_array_info_t to grow a "journal_disks" attribute.
^ permalink raw reply
* Re: Unable to assemble RAID6 after Ubuntu>Arch switch
From: Mathias Burén @ 2015-10-06 22:10 UTC (permalink / raw)
To: Phil Turmel; +Cc: Alexander Afonyashin, Linux-RAID
In-Reply-To: <CADNH=7H9DcEFEEOmcMjn1fS-2efPB_eXVN8zxCoVCHRaCpLR3Q@mail.gmail.com>
I got it working by following Alexander's suggestion (boot back into
Ubuntu, fail a device, remove it, clear all superblocks on it, re-add
it, repeat for every device with traces of the old array). Archlinux
now detects everything automatically on boot.
Thank you!
Mathias
On 26 September 2015 at 19:01, Mathias Burén <mathias.buren@gmail.com> wrote:
> On 26 September 2015 at 12:33, Phil Turmel <philip@turmel.org> wrote:
>> On 09/26/2015 08:16 AM, Mathias Burén wrote:
>>> Hi,
>>>
>>> Would it be possible to somehow manually assemble array under Arch?
>>> Since the Arch kernel doesn't see /dev/sda1 and /dev/sdb1 (fdisk sees
>>> them but they do not appear in /dev) I can't assemble arrays.
>>> partprobe doesn't help. Any ideas why the partitions wouldn't populate in /dev ?
>>
>> Is there any chance you've managed to get a GPT label onto those disks?
>> And your arch kernel has omitted GPT support?
>>
>> Phil
>>
>
> Good idea Phil, but it's enabled by default:
>
> $ zgrep CONFIG_EFI_PARTITION /proc/config.gz
> CONFIG_EFI_PARTITION=y
--
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 3/6] Enable create array with write journal (--write-journal DEVICE).
From: Song Liu @ 2015-10-07 6:06 UTC (permalink / raw)
To: Dan Williams; +Cc: linux-raid, Shaohua Li, Neil Brown, Christoph Hellwig
In-Reply-To: <CAPcyv4g8b=RS7um6KzpmfrEtF1WaC2DF050gDNdA7MxP6mh-nw@mail.gmail.com>
> -----Original Message-----
> From: Dan Williams [mailto:dan.j.williams@intel.com]
> Sent: Tuesday, October 6, 2015 11:32 AM
> To: Song Liu <songliubraving@fb.com>
> Cc: linux-raid <linux-raid@vger.kernel.org>; Shaohua Li <shli@fb.com>; Neil
> Brown <neilb@suse.de>; Christoph Hellwig <hch@infradead.org>
> Subject: Re: [PATCH 3/6] Enable create array with write journal (--write-
> journal DEVICE).
>
> On Fri, Aug 28, 2015 at 4:27 PM, Song Liu <songliubraving@fb.com> wrote:
> > Specify the write journal device with --write-journal DEVICE
> >
> > ./mdadm --create -f /dev/md0 --assume-clean -c 32 --raid-devices=4
> > --level=5 /dev/sd[c-f] --write-journal /dev/sdb1
> > mdadm: Defaulting to version 1.2 metadata
> > mdadm: array /dev/md0 started.
> >
> > Only one journal device is allowed. If multiple --write-journal are
> > given, mdadm will use the first and ignore others
> >
> > ./mdadm --create -f /dev/md0 --assume-clean -c 32 --raid-devices=4
> > --level=5 /dev/sd[c-f] --write-journal /dev/sdb1 --write-journal
> > /dev/sdx
> > mdadm: Please specify only one journal device for the array.
> > mdadm: Ignoring --write-journal /dev/sdx...
> > mdadm: Defaulting to version 1.2 metadata
> > mdadm: array /dev/md0 started.
>
> Hmm, perhaps this should instead follow the same command line mechanics
> as "--write-mostly" whereby all the devices listed after that flag take on that
> role. Also, if we anticipate more roles I wonder if the option should be "--
> role=" that assigns the given role to the device(s) that follow.
>
> Thoughts?
>
> I'm looking at this from the perspective of reviving the SRT patches [1] where
> a role for a read/write cache device is needed. Where a read/write-cache is
> different than a journal.
>
> [1]:
> https://urldefense.proofpoint.com/v1/url?u=https://lwn.net/Articles/59661
> 4/&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=J3jGe56dIPfS5TN6DM82U
> YbbeR1j2viaiSJI40tv6lE%3D%0A&m=dv5znFd%2BTZdHZaSKjzBVqJbdtNBALR0
> RgzhZsOmsWAU%3D%0A&s=80c812981ff98264cf1bb2b79032863d4e33ba100
> 093d41956eab3fbd33b5635
I think --write-mostly is used as _hint_ to RAID 1 that differentiates SSD from
HDD. While --write-journal turns on "MD_FEATURE_JOURNAL" and specifies
journal device. We should not need more than 1 journal device. In this sense,
I think current --write-journal works well...
Thanks,
Song
^ permalink raw reply
* RE: [PATCH 4/6] Assemble array with write journal
From: Song Liu @ 2015-10-07 6:13 UTC (permalink / raw)
To: Dan Williams; +Cc: linux-raid, Shaohua Li, Neil Brown, Christoph Hellwig
In-Reply-To: <CAPcyv4j=8SS0Hfwu3AG3iQ+pCrXZMheMU1HM2L_LZXkNJY48wg@mail.gmail.com>
> -----Original Message-----
> From: Dan Williams [mailto:dan.j.williams@intel.com]
> Sent: Tuesday, October 6, 2015 1:12 PM
> To: Song Liu <songliubraving@fb.com>
> Cc: linux-raid <linux-raid@vger.kernel.org>; Shaohua Li <shli@fb.com>; Neil
> Brown <neilb@suse.de>; Christoph Hellwig <hch@infradead.org>
> Subject: Re: [PATCH 4/6] Assemble array with write journal
>
> On Fri, Aug 28, 2015 at 4:27 PM, Song Liu <songliubraving@fb.com> wrote:
> > Example output:
> >
> > ./mdadm --assemble /dev/md0 /dev/sd[c-f] /dev/sdb1
> > mdadm: /dev/md0 has been started with 4 drives and 1 journal.
> >
> > mdadm checks superblock for journal devices. If the array appears to
> > have a journal device, but it is not given, it will complain as
> >
> > ./mdadm --assemble /dev/md0 /dev/sd[c-f]
> > mdadm: Not safe to assemble with journal device missing, consider --force.
> >
> > This can be overwritten with --force
> >
> > ./mdadm --assemble /dev/md0 /dev/sd[c-f] --force
> > mdadm: Force start with missing journal device...
> > mdadm: /dev/md0 has been started with 4 drives.
> >
> > Signed-off-by: Shaohua Li <shli@fb.com>
> > Signed-off-by: Song Liu <songliubraving@fb.com>
> [..]
> > diff --git a/mdadm.h b/mdadm.h
> > index 3cc1532..bc6680f 100644
> > --- a/mdadm.h
> > +++ b/mdadm.h
> > @@ -969,6 +969,9 @@ extern struct superswitch {
> > /* validate container after assemble */
> > int (*validate_container)(struct mdinfo *info);
> >
> > + /* whether the array require a journal device */
> > + int (*require_journal)(struct supertype *st);
> > +
> > int swapuuid; /* true if uuid is bigending rather than hostendian */
> > int external;
> > const char *name; /* canonical metadata name */
>
> I'm wondering why is this a function pointer rather than a flag? Is it because
> "roles" are only an internal detail of v1 superblocks?
> Might be time to make them a first class citizen. Again, I would use that
> generic "roles" enabling for a new rev of the SRT patches.
Actually, I am planning to have require_journal1 just read
MD_FEATURE_JOURNAL bit. But the feature_map is super 1 only. Not sure
whether there is another flag for this.
Thanks,
Song
^ permalink raw reply
* 30c2a1faaeb3db94fc92f79553cc72634aa3b218 broke cryptsetup on my machine
From: Giedrius Statkevičius @ 2015-10-07 19:27 UTC (permalink / raw)
To: geliangtang, snitzer; +Cc: linux-raid, linux-kernel
Hello,
I have a LUKS on LVM setup: /boot is unencrypted and everything is "hidden" in
/dev/sda2. After booting on linux-next and just after entering my password
cryptsetup segfaults and a stack trace is printed from the kernel. Since writing
down all those numbers is hard I've made a picture where it is shown:
https://i.imgur.com/6PHNUdv.jpg
I figured it had something to do with changes to memory management and thus find
out that commit 30c2a1faaeb3db94fc92f79553cc72634aa3b218 broke my system.
Reverting it there is no such error anymore and system boots fine past that
point.
Could someone look into this? Thanks, Giedrius
^ permalink raw reply
* Re: [PATCH 3/6] raid5-cache: add trim support for log
From: Neil Brown @ 2015-10-08 1:53 UTC (permalink / raw)
To: Shaohua Li, linux-raid; +Cc: Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <581084e9c46ee200f8fedf2dcfec4e897517c0ed.1443973492.git.shli@fb.com>
[-- Attachment #1: Type: text/plain, Size: 2842 bytes --]
Shaohua Li <shli@fb.com> writes:
> Since superblock is updated infrequently, we do a simple trim of log
> disk (a synchronous trim)
>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
> drivers/md/raid5-cache.c | 38 +++++++++++++++++++++++++++++++++++++-
> 1 file changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
> index 93097a8..6c52168 100644
> --- a/drivers/md/raid5-cache.c
> +++ b/drivers/md/raid5-cache.c
> @@ -654,6 +654,42 @@ static void r5l_kick_io_unit(struct r5l_log *log)
> }
>
> static void r5l_write_super(struct r5l_log *log, sector_t cp);
> +static void r5l_write_super_and_discard_space(struct r5l_log *log,
> + sector_t end)
> +{
> + struct block_device *bdev = log->rdev->bdev;
> + struct mddev *mddev;
> +
> + r5l_write_super(log, end);
> +
> + if (!blk_queue_discard(bdev_get_queue(bdev)))
> + return;
> +
> + mddev = log->rdev->mddev;
> + if (!mddev_is_locked(mddev)) {
> + set_bit(MD_CHANGE_PENDING, &mddev->flags);
> + md_wakeup_thread(mddev->thread);
> + wait_event(mddev->sb_wait,
> + !test_bit(MD_CHANGE_PENDING, &mddev->flags));
> + } else { /* we are stopping the array, already take reconfig_mutex */
> + md_update_sb(mddev, 1);
> + }
No.
Just because mddev is locked, that doesn't mean that this thread owns
the lock. Some other thread might just happen to have it locked for a
moment, and may unlock it long before we get to md_update_sb().
If you cannot block here, then you need to find a way to schedule the
discard after the md_update_sb() has happened.
Maybe set some flag, and in raid5d(), after md_check_recovery(), see if
the superblock has been written and if the discard is still pending, and
then do the discard. Maybe.
Or pass a flag into r5l_do_reclaim() to say whether this thread holds
the lock or not.
NeilBrown
> +
> + if (log->last_checkpoint < end) {
> + blkdev_issue_discard(bdev,
> + log->last_checkpoint + log->rdev->data_offset,
> + end - log->last_checkpoint, GFP_NOIO, 0);
> + } else {
> + blkdev_issue_discard(bdev,
> + log->last_checkpoint + log->rdev->data_offset,
> + log->device_size - log->last_checkpoint,
> + GFP_NOIO, 0);
> + blkdev_issue_discard(bdev, log->rdev->data_offset, end,
> + GFP_NOIO, 0);
> + }
> +}
> +
> +
> static void r5l_do_reclaim(struct r5l_log *log)
> {
> struct r5l_io_unit *io, *last;
> @@ -709,7 +745,7 @@ static void r5l_do_reclaim(struct r5l_log *log)
> * here, because the log area might be reused soon and we don't want to
> * confuse recovery
> */
> - r5l_write_super(log, last->log_start);
> + r5l_write_super_and_discard_space(log, last->log_start);
>
> mutex_lock(&log->io_mutex);
> log->last_checkpoint = last->log_start;
> --
> 2.4.6
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: [PATCH 4/6] md: don't export log device
From: Neil Brown @ 2015-10-08 1:57 UTC (permalink / raw)
To: Shaohua Li, linux-raid; +Cc: Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <7aecea0895b46e28c2b8a876a83da10d7f30d7f1.1443973492.git.shli@fb.com>
[-- Attachment #1: Type: text/plain, Size: 1905 bytes --]
Shaohua Li <shli@fb.com> writes:
> If there is IO error in log device, don't export it like other raid
> disks. Otherwise we get kernel crash in different places since
> rdev->bdev, rdev->mddev becomes NULL
>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
> drivers/md/md.c | 4 ++--
> drivers/md/raid5.c | 2 ++
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index c643c9a..ec6574d 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -2523,7 +2523,7 @@ state_store(struct md_rdev *rdev, const char *buf, size_t len)
> else
> err = -EBUSY;
> } else if (cmd_match(buf, "remove")) {
> - if (rdev->raid_disk >= 0)
> + if (rdev->raid_disk >= 0 || test_bit(Journal, &rdev->flags))
> err = -EBUSY;
> else {
> struct mddev *mddev = rdev->mddev;
> @@ -6044,7 +6044,7 @@ static int hot_remove_disk(struct mddev *mddev, dev_t dev)
> clear_bit(Blocked, &rdev->flags);
> remove_and_add_spares(mddev, rdev);
>
> - if (rdev->raid_disk >= 0)
> + if (rdev->raid_disk >= 0 || test_bit(Journal, &rdev->flags))
> goto busy;
>
> if (mddev_is_clustered(mddev))
Neither of these chunks should be needed.
->raid_disk of an active devices is only set to -1 if ->hot_remove_disk
succeeds.
You have make ->hot_remove_disk fail for Journal devices, so ->raid_disk
will be >= 0.
NeilBrown
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 216fa3c..c164501 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -7128,6 +7128,8 @@ static int raid5_remove_disk(struct mddev *mddev, struct md_rdev *rdev)
> struct disk_info *p = conf->disks + number;
>
> print_raid5_conf(conf);
> + if (test_bit(Journal, &rdev->flags))
> + return -EBUSY;
> if (rdev == p->rdev)
> rdevp = &p->rdev;
> else if (rdev == p->replacement)
> --
> 2.4.6
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: [PATCH 0/6] raid5-cache fixes
From: Neil Brown @ 2015-10-08 2:10 UTC (permalink / raw)
To: Shaohua Li, linux-raid; +Cc: Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <cover.1443973492.git.shli@fb.com>
[-- Attachment #1: Type: text/plain, Size: 1536 bytes --]
Shaohua Li <shli@fb.com> writes:
> Hi,
>
> some fixes for raid5-cache.
> patch 1, a small fix
> patch 2-3, move reclaim teardown to quiesce handling and add trim support. I
> still need md_update_sb there and play the mddev_is_locked trick as the
> reconfig_mutex is already hold at md stop. The md_update_sb probably can move
> to md core later.
> patch 4-6, add error handling. For patch 4, I still need the journal bit check
> in in md core, otherwise there is no way to prevent 'echo remove > rdev/state'
> to delete journal disk. For patch 6, I didn't change has_failed() yet. Handling
> assemble with miss/failed log disk is still on going.
>
> Next step is to make assemble correct with miss/failed log disk. This will need
> kernel/utilities cooperation. Song and I are working on it.
>
> Thanks,
> Shaohua
>
> Shaohua Li (6):
> md: show journal for journal disk in disk state sysfs
> raid5-cache: move reclaim stop to quiesce
> raid5-cache: add trim support for log
> md: don't export log device
> md: set In_Sync for log disk
> raid5-cache: IO error handling
>
> drivers/md/md.c | 9 ++++--
> drivers/md/raid5-cache.c | 83 ++++++++++++++++++++++++++++++++++++++++--------
> drivers/md/raid5.c | 7 +++-
> drivers/md/raid5.h | 3 ++
> 4 files changed, 85 insertions(+), 17 deletions(-)
>
> --
> 2.4.6
Thanks.
I've applied 1, 2, and 5. Should appear in 'devel' shortly.
3 and 4 I've replies to separately, 6 depends on 3.
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: raid5-cache I/O path improvements V3
From: Neil Brown @ 2015-10-08 2:45 UTC (permalink / raw)
To: Shaohua Li, Christoph Hellwig; +Cc: dan.j.williams, Kernel-team, linux-raid
In-Reply-To: <20151006181335.GA3367313@devbig084.prn1.facebook.com>
[-- Attachment #1: Type: text/plain, Size: 777 bytes --]
Shaohua Li <shli@fb.com> writes:
> On Mon, Oct 05, 2015 at 09:31:05AM +0200, Christoph Hellwig wrote:
>> Hi Shaohua, hi Neil,
>>
>> this series contains a few updates to the raid5-cache feature, and goes
>> on top of dm/devel + the series Shaohua sent yesterday.
>>
>> Besides the rebase the only changes compared to the previous version
>> is that we only bypasss the batch and flush state for devices that
>> do not have a volatile cache at all like persistent memory or high
>> performance flash devices.
>
> Thanks Christoph, patches look great.
>
> Reviewed-by: Shaohua Li <shli@fb.com>
Thanks. I've applied all these (fixing up minor conflict due to an
earlier patch I didn't accept yet). They are all in my 'devel' branch.
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: [PATCH 0/6] raid5-cache fixes
From: Shaohua Li @ 2015-10-08 2:56 UTC (permalink / raw)
To: Neil Brown; +Cc: linux-raid, Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <87mvvu6rfr.fsf@notabene.neil.brown.name>
On Thu, Oct 08, 2015 at 01:10:32PM +1100, Neil Brown wrote:
> Shaohua Li <shli@fb.com> writes:
>
> > Hi,
> >
> > some fixes for raid5-cache.
> > patch 1, a small fix
> > patch 2-3, move reclaim teardown to quiesce handling and add trim support. I
> > still need md_update_sb there and play the mddev_is_locked trick as the
> > reconfig_mutex is already hold at md stop. The md_update_sb probably can move
> > to md core later.
> > patch 4-6, add error handling. For patch 4, I still need the journal bit check
> > in in md core, otherwise there is no way to prevent 'echo remove > rdev/state'
> > to delete journal disk. For patch 6, I didn't change has_failed() yet. Handling
> > assemble with miss/failed log disk is still on going.
> >
> > Next step is to make assemble correct with miss/failed log disk. This will need
> > kernel/utilities cooperation. Song and I are working on it.
> >
> > Thanks,
> > Shaohua
> >
> > Shaohua Li (6):
> > md: show journal for journal disk in disk state sysfs
> > raid5-cache: move reclaim stop to quiesce
> > raid5-cache: add trim support for log
> > md: don't export log device
> > md: set In_Sync for log disk
> > raid5-cache: IO error handling
> >
> > drivers/md/md.c | 9 ++++--
> > drivers/md/raid5-cache.c | 83 ++++++++++++++++++++++++++++++++++++++++--------
> > drivers/md/raid5.c | 7 +++-
> > drivers/md/raid5.h | 3 ++
> > 4 files changed, 85 insertions(+), 17 deletions(-)
> >
> > --
> > 2.4.6
>
> Thanks.
>
> I've applied 1, 2, and 5. Should appear in 'devel' shortly.
> 3 and 4 I've replies to separately, 6 depends on 3.
please hold 5 currently. I'm a little confused about the In_sync bit.
It's quite tricky to handle this bit. For example, if both In_sync and
Journal bits are set, I'll need move checking the 'Journal' bit ahead of
checking the 'In_sync' in super_1_sync (current patch haven't done it
yet, it's a bug). There are similar cases in mdadm too. This makes me
thinking about What's the exact role for the In_sync bit for journal
disk. The comments in the bit definition doesn't give an answer. We can
use the Faulty bit for error handling. Any thoughts?
Thanks,
Shaohua
^ permalink raw reply
* Re: [PATCH 4/6] md: don't export log device
From: Shaohua Li @ 2015-10-08 3:16 UTC (permalink / raw)
To: Neil Brown; +Cc: linux-raid, Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <87pp0q6s1i.fsf@notabene.neil.brown.name>
On Thu, Oct 08, 2015 at 12:57:29PM +1100, Neil Brown wrote:
> Shaohua Li <shli@fb.com> writes:
>
> > If there is IO error in log device, don't export it like other raid
> > disks. Otherwise we get kernel crash in different places since
> > rdev->bdev, rdev->mddev becomes NULL
> >
> > Signed-off-by: Shaohua Li <shli@fb.com>
> > ---
> > drivers/md/md.c | 4 ++--
> > drivers/md/raid5.c | 2 ++
> > 2 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index c643c9a..ec6574d 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -2523,7 +2523,7 @@ state_store(struct md_rdev *rdev, const char *buf, size_t len)
> > else
> > err = -EBUSY;
> > } else if (cmd_match(buf, "remove")) {
> > - if (rdev->raid_disk >= 0)
> > + if (rdev->raid_disk >= 0 || test_bit(Journal, &rdev->flags))
> > err = -EBUSY;
> > else {
> > struct mddev *mddev = rdev->mddev;
> > @@ -6044,7 +6044,7 @@ static int hot_remove_disk(struct mddev *mddev, dev_t dev)
> > clear_bit(Blocked, &rdev->flags);
> > remove_and_add_spares(mddev, rdev);
> >
> > - if (rdev->raid_disk >= 0)
> > + if (rdev->raid_disk >= 0 || test_bit(Journal, &rdev->flags))
> > goto busy;
> >
> > if (mddev_is_clustered(mddev))
>
> Neither of these chunks should be needed.
> ->raid_disk of an active devices is only set to -1 if ->hot_remove_disk
> succeeds.
> You have make ->hot_remove_disk fail for Journal devices, so ->raid_disk
> will be >= 0.
I agree the raid5_remove_disk part is superficial, I fixed in an updated
patch. I still didn't get the point what can prevent a journal disk is
removed. Currently the raid_disk is always -1 for journal disk. If it
should be >=0, what value it should be? We give journal disk a special
role '0xfffd' currently.
Thanks,
shaohua
>
> > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> > index 216fa3c..c164501 100644
> > --- a/drivers/md/raid5.c
> > +++ b/drivers/md/raid5.c
> > @@ -7128,6 +7128,8 @@ static int raid5_remove_disk(struct mddev *mddev, struct md_rdev *rdev)
> > struct disk_info *p = conf->disks + number;
> >
> > print_raid5_conf(conf);
> > + if (test_bit(Journal, &rdev->flags))
> > + return -EBUSY;
> > if (rdev == p->rdev)
> > rdevp = &p->rdev;
> > else if (rdev == p->replacement)
> > --
> > 2.4.6
^ permalink raw reply
* Re: [PATCH 0/6] raid5-cache fixes
From: Neil Brown @ 2015-10-08 3:18 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid, Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <20151008025603.GA3007253@devbig084.prn1.facebook.com>
[-- Attachment #1: Type: text/plain, Size: 1296 bytes --]
Shaohua Li <shli@fb.com> writes:
>
> please hold 5 currently. I'm a little confused about the In_sync bit.
> It's quite tricky to handle this bit. For example, if both In_sync and
> Journal bits are set, I'll need move checking the 'Journal' bit ahead of
> checking the 'In_sync' in super_1_sync (current patch haven't done it
> yet, it's a bug). There are similar cases in mdadm too. This makes me
> thinking about What's the exact role for the In_sync bit for journal
> disk. The comments in the bit definition doesn't give an answer. We can
> use the Faulty bit for error handling. Any thoughts?
"In_sync" effectively means that recovery_offset == MaxSector. It means
all data which should be on the device is on the device (except as
described in the bad-block log).
It is set at array-creation time or when recovery completes, and is
cleared when an error is detected. It is useful for differentiating
between a spare being added (without In_sync) or a recently failed
device being re-added (with In_sync).
None of this really relates to the Journal. So as you say, it doesn't
mean much to set that flag for the log.
Maybe r5l_log_disk_error() should check Error rather than In_sync. Was
there a reason you didn't do that in the first place?
I'll drop that patch.
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: [PATCH 0/6] raid5-cache fixes
From: Shaohua Li @ 2015-10-08 3:24 UTC (permalink / raw)
To: Neil Brown; +Cc: linux-raid, Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <87egh66oas.fsf@notabene.neil.brown.name>
On Thu, Oct 08, 2015 at 02:18:19PM +1100, Neil Brown wrote:
> Shaohua Li <shli@fb.com> writes:
> >
> > please hold 5 currently. I'm a little confused about the In_sync bit.
> > It's quite tricky to handle this bit. For example, if both In_sync and
> > Journal bits are set, I'll need move checking the 'Journal' bit ahead of
> > checking the 'In_sync' in super_1_sync (current patch haven't done it
> > yet, it's a bug). There are similar cases in mdadm too. This makes me
> > thinking about What's the exact role for the In_sync bit for journal
> > disk. The comments in the bit definition doesn't give an answer. We can
> > use the Faulty bit for error handling. Any thoughts?
>
> "In_sync" effectively means that recovery_offset == MaxSector. It means
> all data which should be on the device is on the device (except as
> described in the bad-block log).
> It is set at array-creation time or when recovery completes, and is
> cleared when an error is detected. It is useful for differentiating
> between a spare being added (without In_sync) or a recently failed
> device being re-added (with In_sync).
>
> None of this really relates to the Journal. So as you say, it doesn't
> mean much to set that flag for the log.
>
> Maybe r5l_log_disk_error() should check Error rather than In_sync. Was
> there a reason you didn't do that in the first place?
Thanks. No, likely a misuse.
^ permalink raw reply
* Re: [PATCH 4/6] md: don't export log device
From: Neil Brown @ 2015-10-08 4:16 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid, Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <20151008031644.GA3063621@devbig084.prn1.facebook.com>
[-- Attachment #1: Type: text/plain, Size: 1045 bytes --]
Shaohua Li <shli@fb.com> writes:
>>
>> Neither of these chunks should be needed.
>> ->raid_disk of an active devices is only set to -1 if ->hot_remove_disk
>> succeeds.
>> You have make ->hot_remove_disk fail for Journal devices, so ->raid_disk
>> will be >= 0.
>
> I agree the raid5_remove_disk part is superficial, I fixed in an updated
> patch. I still didn't get the point what can prevent a journal disk is
> removed. Currently the raid_disk is always -1 for journal disk. If it
> should be >=0, what value it should be? We give journal disk a special
> role '0xfffd' currently.
Oh, are we leaving the ->raid_disk at -1 for the journal? I hadn't
noticed that. I don't feel comfortable it. Too much code assumes that
<0 means "not in use".
Probably set it to 0, and add a check to setup_conf(), and adjust the
check in run(). md_update_sb() probably need to be careful of journals
too (to not change ->recovery_offset).
I wonder what 'slot_show' should report for the journal.... maybe
"journal"??
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: [PATCH RESEND] imsm: don't call abort_reshape() in imsm_manage_reshape()
From: Neil Brown @ 2015-10-08 4:23 UTC (permalink / raw)
Cc: linux-raid, Artur Paszkiewicz, Konrad Dabrowski
In-Reply-To: <56127893.6070606@intel.com>
[-- Attachment #1: Type: text/plain, Size: 1018 bytes --]
Artur Paszkiewicz <artur.paszkiewicz@intel.com> writes:
> Calling abort_reshape() in imsm_manage_reshape() is unnecessary in case
> of an error because it is handled by reshape_array(). Calling it when
> reshape completes successfully is also unnecessary and leads to a race
> condition:
> - reshape ends
> - mdadm calls abort_reshape() -> sets sync_action to idle
> - MD_RECOVERY_INTR is set and md_reap_sync_thread() does not finish the
> reshape
>
> Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
> Signed-off-by: Konrad Dabrowski <konrad.dabrowski@intel.com>
> ---
> super-intel.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/super-intel.c b/super-intel.c
> index 95a72b6..e609e0c 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -10601,7 +10601,6 @@ static int imsm_manage_reshape(
> ret_val = 1;
> abort:
> free(buf);
> - abort_reshape(sra);
>
> return ret_val;
> }
> --
> 2.1.4
Thanks for the resend.
Applied now.
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: [PATCH 4/6] md: don't export log device
From: Shaohua Li @ 2015-10-08 4:31 UTC (permalink / raw)
To: Neil Brown; +Cc: linux-raid, Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <87bnca6lli.fsf@notabene.neil.brown.name>
On Thu, Oct 08, 2015 at 03:16:41PM +1100, Neil Brown wrote:
> Shaohua Li <shli@fb.com> writes:
> >>
> >> Neither of these chunks should be needed.
> >> ->raid_disk of an active devices is only set to -1 if ->hot_remove_disk
> >> succeeds.
> >> You have make ->hot_remove_disk fail for Journal devices, so ->raid_disk
> >> will be >= 0.
> >
> > I agree the raid5_remove_disk part is superficial, I fixed in an updated
> > patch. I still didn't get the point what can prevent a journal disk is
> > removed. Currently the raid_disk is always -1 for journal disk. If it
> > should be >=0, what value it should be? We give journal disk a special
> > role '0xfffd' currently.
>
> Oh, are we leaving the ->raid_disk at -1 for the journal? I hadn't
> noticed that. I don't feel comfortable it. Too much code assumes that
> <0 means "not in use".
>
> Probably set it to 0, and add a check to setup_conf(), and adjust the
> check in run(). md_update_sb() probably need to be careful of journals
> too (to not change ->recovery_offset).
> I wonder what 'slot_show' should report for the journal.... maybe
> "journal"??
->raid_disk >= 0 is for normal raid disks. If we use it, we will have
two disks with ->raid_disk 0, it sounds weird. Currently we add the
'test(Journal, rdev->flags)' check in different places to destinguish
journal disk. We will need to audit the code which assumes ' < 0 means
not in use'. We will probably need to audit the same code if we set
->raid_disk 0 for journal. Neither is perfect.
Thanks,
Shaohua
^ permalink raw reply
* Re: [PATCH 4/6] md: don't export log device
From: Neil Brown @ 2015-10-08 6:04 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid, Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <20151008043121.GA3189627@devbig084.prn1.facebook.com>
[-- Attachment #1: Type: text/plain, Size: 2246 bytes --]
Shaohua Li <shli@fb.com> writes:
> On Thu, Oct 08, 2015 at 03:16:41PM +1100, Neil Brown wrote:
>> Shaohua Li <shli@fb.com> writes:
>> >>
>> >> Neither of these chunks should be needed.
>> >> ->raid_disk of an active devices is only set to -1 if ->hot_remove_disk
>> >> succeeds.
>> >> You have make ->hot_remove_disk fail for Journal devices, so ->raid_disk
>> >> will be >= 0.
>> >
>> > I agree the raid5_remove_disk part is superficial, I fixed in an updated
>> > patch. I still didn't get the point what can prevent a journal disk is
>> > removed. Currently the raid_disk is always -1 for journal disk. If it
>> > should be >=0, what value it should be? We give journal disk a special
>> > role '0xfffd' currently.
>>
>> Oh, are we leaving the ->raid_disk at -1 for the journal? I hadn't
>> noticed that. I don't feel comfortable it. Too much code assumes that
>> <0 means "not in use".
>>
>> Probably set it to 0, and add a check to setup_conf(), and adjust the
>> check in run(). md_update_sb() probably need to be careful of journals
>> too (to not change ->recovery_offset).
>> I wonder what 'slot_show' should report for the journal.... maybe
>> "journal"??
>
> ->raid_disk >= 0 is for normal raid disks. If we use it, we will have
> two disks with ->raid_disk 0, it sounds weird. Currently we add the
> 'test(Journal, rdev->flags)' check in different places to destinguish
> journal disk. We will need to audit the code which assumes ' < 0 means
> not in use'. We will probably need to audit the same code if we set
> ->raid_disk 0 for journal. Neither is perfect.
Having two disks with ->raid_disk==0 does seem a little weird, but we do
already have that in some cases.
When you have a hot-replace going, both the original and the replacement
have the same ->raid_disk numbers. They can be distinguished by the
Replacement flag.
I'm suggesting the same (sort of) for journals, and distinguish by the
Journal flag.
I did quick audit and just found setup_conf, run() and md_update_sb().
If you could do an audit to that would be good. I'd be surprised if you
find many more places where Journal needs to be tested with ->raid_disk.
Thanks,
NeilBrown
>
> Thanks,
> Shaohua
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* [PATCH] dm ioctl: bugfix for kvfree() in free_params()
From: Geliang Tang @ 2015-10-08 11:46 UTC (permalink / raw)
To: Alasdair Kergon, Mike Snitzer, dm-devel, Neil Brown
Cc: Geliang Tang, linux-raid, linux-kernel
The ioctl parameter block consists of two parts, a dm_ioctl struct
followed by a data buffer. Don't free the parameter block when the
data buffer is empty.
Fixes: 30c2a1f('dm ioctl: use kvfree() in free_params()')
Signed-off-by: Geliang Tang <geliangtang@163.com>
---
drivers/md/dm-ioctl.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index dbb5588..3f93787 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -1673,10 +1673,13 @@ static int check_version(unsigned int cmd, struct dm_ioctl __user *user)
static void free_params(struct dm_ioctl *param, size_t param_size, int param_flags)
{
+ const size_t minimum_data_size = sizeof(*param) - sizeof(param->data);
+
if (param_flags & DM_WIPE_BUFFER)
memset(param, 0, param_size);
- kvfree(param);
+ if (param_size > minimum_data_size)
+ kvfree(param);
}
static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kernel,
--
1.9.1
^ permalink raw reply related
* Re: 30c2a1faaeb3db94fc92f79553cc72634aa3b218 broke cryptsetup on my machine
From: Geliang Tang @ 2015-10-08 12:38 UTC (permalink / raw)
To: Giedrius Statkevičius; +Cc: snitzer, linux-raid, linux-kernel
In-Reply-To: <alpine.LNX.2.20.1510072226180.1350@localhost.localdomain>
On Wed, Oct 07, 2015 at 10:27:27PM +0300, Giedrius Statkevičius wrote:
> Hello,
> I have a LUKS on LVM setup: /boot is unencrypted and everything is "hidden" in
> /dev/sda2. After booting on linux-next and just after entering my password
> cryptsetup segfaults and a stack trace is printed from the kernel. Since writing
> down all those numbers is hard I've made a picture where it is shown:
> https://i.imgur.com/6PHNUdv.jpg
>
> I figured it had something to do with changes to memory management and thus find
> out that commit 30c2a1faaeb3db94fc92f79553cc72634aa3b218 broke my system.
> Reverting it there is no such error anymore and system boots fine past that
> point.
>
> Could someone look into this? Thanks, Giedrius
Bugfix for this has been sended out.
https://lkml.org/lkml/2015/10/8/342.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/5] fs: Verify access of user towards block device file when mounting
From: Seth Forshee @ 2015-10-08 15:41 UTC (permalink / raw)
To: Eric W. Biederman, Mike Snitzer
Cc: Kent Overstreet, Alasdair Kergon, dm-devel, Neil Brown,
David Woodhouse, Brian Norris, Alexander Viro, Jan Kara,
Jeff Layton, J. Bruce Fields, Serge Hallyn, Andy Lutomirski,
linux-fsdevel, linux-security-module, selinux, linux-kernel,
linux-mtd, linux-bcache, linux-raid
In-Reply-To: <20151001144137.GC101875@ubuntu-hedt>
On Thu, Oct 01, 2015 at 09:41:37AM -0500, Seth Forshee wrote:
> On Thu, Oct 01, 2015 at 09:40:52AM -0400, Mike Snitzer wrote:
> > On Thu, Oct 01 2015 at 8:55am -0400,
> > Seth Forshee <seth.forshee@canonical.com> wrote:
> >
> > > On Wed, Sep 30, 2015 at 07:42:15PM -0400, Mike Snitzer wrote:
> > > > On Wed, Sep 30 2015 at 4:15pm -0400,
> > > > Seth Forshee <seth.forshee@canonical.com> wrote:
> > > >
> > > > > When mounting a filesystem on a block device there is currently
> > > > > no verification that the user has appropriate access to the
> > > > > device file passed to mount. This has not been an issue so far
> > > > > since the user in question has always been root, but this must
> > > > > be changed before allowing unprivileged users to mount in user
> > > > > namespaces.
> > > > >
> > > > > To fix this, add an argument to lookup_bdev() to specify the
> > > > > required permissions. If the mask of permissions is zero, or
> > > > > if the user has CAP_SYS_ADMIN, the permission check is skipped,
> > > > > otherwise the lookup fails if the user does not have the
> > > > > specified access rights for the inode at the supplied path.
> > > > >
> > > > > Callers associated with mounting are updated to pass permission
> > > > > masks to lookup_bdev() so that these mounts will fail for an
> > > > > unprivileged user who lacks permissions for the block device
> > > > > inode. All other callers pass 0 to maintain their current
> > > > > behaviors.
> > > > >
> > > > > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> > > > > ---
> > > > > drivers/md/bcache/super.c | 2 +-
> > > > > drivers/md/dm-table.c | 2 +-
> > > > > drivers/mtd/mtdsuper.c | 6 +++++-
> > > > > fs/block_dev.c | 18 +++++++++++++++---
> > > > > fs/quota/quota.c | 2 +-
> > > > > include/linux/fs.h | 2 +-
> > > > > 6 files changed, 24 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> > > > > index e76ed003769e..35bb3ea4cbe2 100644
> > > > > --- a/drivers/md/dm-table.c
> > > > > +++ b/drivers/md/dm-table.c
> > > > > @@ -380,7 +380,7 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> > > > > BUG_ON(!t);
> > > > >
> > > > > /* convert the path to a device */
> > > > > - bdev = lookup_bdev(path);
> > > > > + bdev = lookup_bdev(path, 0);
> > > > > if (IS_ERR(bdev)) {
> > > > > dev = name_to_dev_t(path);
> > > > > if (!dev)
> > > >
> > > > Given dm_get_device() is passed @mode why not have it do something like
> > > > you did in blkdev_get_by_path()? e.g.:
> > >
> > > I only dealt with code related to mounting in this patch since that's
> > > what I'm working on. I have it on my TODO list to consider converting
> > > other callers of lookup_bdev. But if you're sure doing so makes sense
> > > for dm_get_device and that it won't cause regressions then I could add a
> > > patch for it.
> >
> > OK, dm_get_device() is called in DM device activation path (by tools
> > like lvm2).
> >
> > After lookup_bdev() it goes on to call blkdev_get_by_dev() with this
> > call chain:
> > dm_get_device -> dm_get_table_device -> open_table_device -> blkdev_get_by_dev
> >
> > Not immediately clear to me why we'd need to augment blkdev_get_by_dev()
> > to do this checking also.
> >
> > However, thinking further: In a device stack (e.g. dm/lvm2, md, etc)
> > new virtual block devices are created that layer ontop of the
> > traditional block devices. This level of indirection may cause your
> > lookup_bdev() check to go on to succeed (if access constraints were not
> > established on the upper level dm or md device?). I'm just thinking
> > outloud here: but have you verified your changes work as intended on
> > devices created with either lvm2 or mdadm?
> >
> > What layer establishes access rights to historically root-only
> > priviledged block devices? Is it user namespaces?
>
> I'm going to start with this last question and work my way backwards.
>
> Who determines access rights to the block devices varies to some degree.
> Any normal user could unshare the user/mount namespaces and still see
> all the block devices in /dev. If we're going to allow that user to then
> mount block devices in their private namespaces, the kernel must verify
> that the user has appropriate permissions for the block device inode.
> That's the point of this patch. In a container context the host process
> which sets up the container might share some block devices into the
> container via bind mounts, in which case it would be responsible for
> setting up access rights (both via inode permissions and potentially via
> devcgroups). I also have some patches I'm working on for a loop psuedo
> filesystem which would allow a container to allocate loop devices for
> its own use (via the loop-control device), in which case the kernel
> creates a device node in the loopfs filesystem from which the new loop
> device was allocated, owned by the root user in the user namespace
> associated with the loopfs superblock.
>
> Obviously a DM block device is more complicated than a traditional block
> device. This patch should ensure that if an unprivileged user tries to
> mount a DM blkdev that it fails if the user lacks permissions for the DM
> blkdev inode. There's also the blkdevs behind the DM blkdev, and I'm not
> sure whether we need to additionally verify that the user has access to
> those devices. Maybe it's enough that someone with access to them set up
> the DM device, and someone with sufficient privileges gave that user
> access to the DM device. Do you have any thoughts?
>
> As for activating a DM device, I assume only root is permitted to do
> this. In that case lookup_bdev will skip the inode permission check
> even if a non-zero permission mask is passed.
I've been looking into this more and doing some testing.
As far as mounting goes, it ends up behaving as I expected. As long as
the user has permission to access the inode for the DM block device, the
device can be mounted. Access to the block devices behind the DM block
device is not required. I think this makes sense - it's consistent with
how read/write access to the DM block device works already independent
of user namespaces.
One thing I noticed that might be a concern (I'm not sure) are ioctls on
DM block devices. It looks to me like often these will be passed through
to the underlying block device.
It appears that all code paths which call dm_get_device originate with
ioctls on /dev/mapper/control. Doing this requires CAP_SYS_ADMIN, in
which case the permission check in lookup_bdev is skipped. So there's no
benefit to having dm_get_device pass a permission mask to
blkdev_get_by_path, nor is there any harm. It will only matter if at
some point userns-root is allowed to set up DM devices.
Seth
^ permalink raw reply
* Re: [PATCH] crash in md-raid1 and md-raid10 due to incorrect list manipulation
From: Neil Brown @ 2015-10-08 21:35 UTC (permalink / raw)
To: Mikulas Patocka; +Cc: linux-raid, dm-devel, linux-kernel, Mike Snitzer
In-Reply-To: <alpine.LRH.2.02.1510011442260.16076@file01.intranet.prod.int.rdu2.redhat.com>
[-- Attachment #1: Type: text/plain, Size: 4204 bytes --]
Mikulas Patocka <mpatocka@redhat.com> writes:
> The commit 55ce74d4bfe1b9444436264c637f39a152d1e5ac (md/raid1: ensure
> device failure recorded before write request returns) is causing crash in
> the LVM2 testsuite test shell/lvchange-raid.sh. For me the crash is 100%
> reproducible.
>
> The reason for the crash is that the newly added code in raid1d moves the
> list from conf->bio_end_io_list to tmp, then tests if tmp is non-empty and
> then incorrectly pops the bio from conf->bio_end_io_list (which is empty
> because the list was alrady moved).
>
> Raid-10 has a similar bug.
Ouch. I can't have been thinking when I wrote that code!
Thanks for finding and fixing this. Patch will be sent to Linus in time
for next -rc.
Thanks,
NeilBrown
>
> Kernel Fault: Code=15 regs=000000006ccb8640 (Addr=0000000100000000)
> CPU: 3 PID: 1930 Comm: mdX_raid1 Not tainted 4.2.0-rc5-bisect+ #35
> task: 000000006cc1f258 ti: 000000006ccb8000 task.ti: 000000006ccb8000
>
> YZrvWESTHLNXBCVMcbcbcbcbOGFRQPDI
> PSW: 00001000000001001111111000001111 Not tainted
> r00-03 000000ff0804fe0f 000000001059d000 000000001059f818 000000007f16be38
> r04-07 000000001059d000 000000007f16be08 0000000000200200 0000000000000001
> r08-11 000000006ccb8260 000000007b7934d0 0000000000000001 0000000000000000
> r12-15 000000004056f320 0000000000000000 0000000000013dd0 0000000000000000
> r16-19 00000000f0d00ae0 0000000000000000 0000000000000000 0000000000000001
> r20-23 000000000800000f 0000000042200390 0000000000000000 0000000000000000
> r24-27 0000000000000001 000000000800000f 000000007f16be08 000000001059d000
> r28-31 0000000100000000 000000006ccb8560 000000006ccb8640 0000000000000000
> sr00-03 0000000000249800 0000000000000000 0000000000000000 0000000000249800
> sr04-07 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>
> IASQ: 0000000000000000 0000000000000000 IAOQ: 000000001059f61c 000000001059f620
> IIR: 0f8010c6 ISR: 0000000000000000 IOR: 0000000100000000
> CPU: 3 CR30: 000000006ccb8000 CR31: 0000000000000000
> ORIG_R28: 000000001059d000
> IAOQ[0]: call_bio_endio+0x34/0x1a8 [raid1]
> IAOQ[1]: call_bio_endio+0x38/0x1a8 [raid1]
> RP(r2): raid_end_bio_io+0x88/0x168 [raid1]
> Backtrace:
> [<000000001059f818>] raid_end_bio_io+0x88/0x168 [raid1]
> [<00000000105a4f64>] raid1d+0x144/0x1640 [raid1]
> [<000000004017fd5c>] kthread+0x144/0x160
>
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Fixes: 55ce74d4bfe1 ("md/raid1: ensure device failure recorded before write request returns.")
> Fixes: 95af587e95aa ("md/raid10: ensure device failure recorded before write request returns.")
>
> ---
> drivers/md/raid1.c | 4 ++--
> drivers/md/raid10.c | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> Index: linux-2.6/drivers/md/raid1.c
> ===================================================================
> --- linux-2.6.orig/drivers/md/raid1.c 2015-10-01 21:10:05.000000000 +0200
> +++ linux-2.6/drivers/md/raid1.c 2015-10-01 21:10:58.000000000 +0200
> @@ -2437,8 +2437,8 @@ static void raid1d(struct md_thread *thr
> }
> spin_unlock_irqrestore(&conf->device_lock, flags);
> while (!list_empty(&tmp)) {
> - r1_bio = list_first_entry(&conf->bio_end_io_list,
> - struct r1bio, retry_list);
> + r1_bio = list_first_entry(&tmp, struct r1bio,
> + retry_list);
> list_del(&r1_bio->retry_list);
> raid_end_bio_io(r1_bio);
> }
> Index: linux-2.6/drivers/md/raid10.c
> ===================================================================
> --- linux-2.6.orig/drivers/md/raid10.c 2015-10-01 21:11:02.000000000 +0200
> +++ linux-2.6/drivers/md/raid10.c 2015-10-01 21:11:19.000000000 +0200
> @@ -2804,8 +2804,8 @@ static void raid10d(struct md_thread *th
> }
> spin_unlock_irqrestore(&conf->device_lock, flags);
> while (!list_empty(&tmp)) {
> - r10_bio = list_first_entry(&conf->bio_end_io_list,
> - struct r10bio, retry_list);
> + r10_bio = list_first_entry(&tmp, struct r10bio,
> + retry_list);
> list_del(&r10_bio->retry_list);
> raid_end_bio_io(r10_bio);
> }
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: Problem w/ commit ac8fa4196d20 on older, slower hardware
From: Neil Brown @ 2015-10-09 0:13 UTC (permalink / raw)
To: Joshua Kinard; +Cc: linux-raid
[-- Attachment #1: Type: text/plain, Size: 2165 bytes --]
> Per commit ac8fa4196d20:
>
> > md: allow resync to go faster when there is competing IO.
> >
> > When md notices non-sync IO happening while it is trying to resync (or
> > reshape or recover) it slows down to the set minimum.
> >
> > The default minimum might have made sense many years ago but the drives have
> > become faster. Changing the default to match the times isn't really a long
> > term solution.
>
> This holds true for modern hardware, but this commit is causing problems on
> older hardware, like SGI MIPS platforms, that use mdraid. Namely, while trying
> to chase down an unrelated hardlock bug on an Onyx2, one of the arrays got out
> of sync, so on the next reboot, mdraid's attempt to resync at full speed
> absolutely murdered interactivity. It took close to 30mins for the system to
> finally reach the login prompt.
>
> Revert this patch was working to mitigate the problem at first, but it appears
> that in recent kernels, this is no longer the case, and reverting this commit
> has no noticeable effect anymore. I assume I'd have to hunt down newer commits
> to revert, but it's probably saner to just highlight the problem and test any
> proposed solutions.
>
> Is there some way to resolve this in such a way that old hardware maintains
> some level of interactivity during a resync, but that won't inconvenience the
> more modern systems?
>
> http://git.linux-mips.org/cgit/ralf/linux.git/commit/?id=ac8fa4196d20
>
> Thanks!,
>
Hmmm... this change shouldn't have that effect.
It should allow resync to soak up a bit more of the idle time, but when
there is any other IO, resync should still back off.
I wonder if there is some other change which has confused the event
counting for the particular hardware you are using.
How did you identify this commit as a possible cause?
The fact that reverting it no longer helps strongly suggests that some
other change is implicated. I don't think there have been other changes
in md which could affect this.
Have you tried adjusting /proc/sys/dev/raid/speed_limit_m{ax,in} ??
Did that have any noticeable effect?
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* [PATCH] md: workqueue: Remove WQ_CPU_INTENSIVE from unbound workqueue allocations
From: Xunlei Pang @ 2015-10-09 3:25 UTC (permalink / raw)
To: linux-kernel
Cc: linux-raid, Alasdair Kergon, Mike Snitzer, Mikulas Patocka,
Neil Brown, Tejun Heo, Xunlei Pang
From: Xunlei Pang <pang.xunlei@linaro.org>
WQ_CPU_INTENSIVE is meaningless for the unbound workqueue, so remove it.
Signed-off-by: Xunlei Pang <pang.xunlei@linaro.org>
---
drivers/md/dm-crypt.c | 4 ++--
drivers/md/dm-verity.c | 3 ++-
drivers/md/raid5.c | 2 +-
3 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 4b3b6f8..56b2560 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -1871,8 +1871,8 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv)
if (test_bit(DM_CRYPT_SAME_CPU, &cc->flags))
cc->crypt_queue = alloc_workqueue("kcryptd", WQ_CPU_INTENSIVE | WQ_MEM_RECLAIM, 1);
else
- cc->crypt_queue = alloc_workqueue("kcryptd", WQ_CPU_INTENSIVE | WQ_MEM_RECLAIM | WQ_UNBOUND,
- num_online_cpus());
+ cc->crypt_queue = alloc_workqueue("kcryptd",
+ WQ_MEM_RECLAIM | WQ_UNBOUND, num_online_cpus());
if (!cc->crypt_queue) {
ti->error = "Couldn't create kcryptd queue";
goto bad;
diff --git a/drivers/md/dm-verity.c b/drivers/md/dm-verity.c
index edc624b..f2fc693 100644
--- a/drivers/md/dm-verity.c
+++ b/drivers/md/dm-verity.c
@@ -942,7 +942,8 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
ti->per_bio_data_size = roundup(sizeof(struct dm_verity_io) + v->shash_descsize + v->digest_size * 2, __alignof__(struct dm_verity_io));
/* WQ_UNBOUND greatly improves performance when running on ramdisk */
- v->verify_wq = alloc_workqueue("kverityd", WQ_CPU_INTENSIVE | WQ_MEM_RECLAIM | WQ_UNBOUND, num_online_cpus());
+ v->verify_wq = alloc_workqueue("kverityd",
+ WQ_MEM_RECLAIM | WQ_UNBOUND, num_online_cpus());
if (!v->verify_wq) {
ti->error = "Cannot allocate workqueue";
r = -ENOMEM;
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 49bb8d3..2a25af8 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -7840,7 +7840,7 @@ static struct md_personality raid4_personality =
static int __init raid5_init(void)
{
raid5_wq = alloc_workqueue("raid5wq",
- WQ_UNBOUND|WQ_MEM_RECLAIM|WQ_CPU_INTENSIVE|WQ_SYSFS, 0);
+ WQ_UNBOUND | WQ_MEM_RECLAIM | WQ_SYSFS, 0);
if (!raid5_wq)
return -ENOMEM;
register_md_personality(&raid6_personality);
--
1.9.1
^ permalink raw reply related
* [PATCH 0/9] raid5-cache fixes
From: Shaohua Li @ 2015-10-09 4:54 UTC (permalink / raw)
To: linux-raid; +Cc: Kernel-team, songliubraving, hch, dan.j.williams, neilb
Hi,
some fixes for raid5-cache/md. Mainly supports trim, handle IO error and
handling array assemble with missing/faulty disks.
patch 1: fix a bug
patch 2: support trim
patch 3-4, IO error handling
patch 5-9, handle assemble issue when journal disk is missing/faulty
If there is IO error to journal disk, write IO will fail immediately. If we
assemble an array with journal feature enabled but with journal disk
missing/faulty, the raid array will start readonly.
Thanks,
Shaohua
Shaohua Li (6):
MD: fix info output for journal disk
raid5-cache: add trim support for log
raid5: journal disk can't be removed
raid5-cache: IO error handling
raid5-cache: start raid5 readonly if journal is missing
MD: set journal disk ->raid_disk
Song Liu (3):
MD: add new bit to indicate raid array with journal
MD: kick out journal disk if it's not fresh
MD: when RAID journal is missing/faulty, block RESTART_ARRAY_RW
drivers/md/md.c | 77 +++++++++++++++++++++++++++++++++++++---------
drivers/md/md.h | 1 +
drivers/md/raid5-cache.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++--
drivers/md/raid5.c | 29 ++++++++++++++++--
drivers/md/raid5.h | 2 ++
5 files changed, 169 insertions(+), 19 deletions(-)
--
2.4.6
^ permalink raw reply
* [PATCH 1/9] MD: fix info output for journal disk
From: Shaohua Li @ 2015-10-09 4:54 UTC (permalink / raw)
To: linux-raid; +Cc: Kernel-team, songliubraving, hch, dan.j.williams, neilb
In-Reply-To: <cover.1444360850.git.shli@fb.com>
journal disk can be faulty. The Journal and Faulty aren't exclusive with
each other.
Signed-off-by: Shaohua Li <shli@fb.com>
---
drivers/md/md.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 0729cc7..daf42bb 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -5857,7 +5857,8 @@ static int get_disk_info(struct mddev *mddev, void __user * arg)
else if (test_bit(In_sync, &rdev->flags)) {
info.state |= (1<<MD_DISK_ACTIVE);
info.state |= (1<<MD_DISK_SYNC);
- } else if (test_bit(Journal, &rdev->flags))
+ }
+ if (test_bit(Journal, &rdev->flags))
info.state |= (1<<MD_DISK_JOURNAL);
if (test_bit(WriteMostly, &rdev->flags))
info.state |= (1<<MD_DISK_WRITEMOSTLY);
@@ -7335,18 +7336,21 @@ static int md_seq_show(struct seq_file *seq, void *v)
rcu_read_lock();
rdev_for_each_rcu(rdev, mddev) {
char b[BDEVNAME_SIZE];
+ bool skip = false;
seq_printf(seq, " %s[%d]",
bdevname(rdev->bdev,b), rdev->desc_nr);
if (test_bit(WriteMostly, &rdev->flags))
seq_printf(seq, "(W)");
if (test_bit(Faulty, &rdev->flags)) {
seq_printf(seq, "(F)");
- continue;
+ skip = true;
}
if (test_bit(Journal, &rdev->flags)) {
seq_printf(seq, "(J)");
- continue;
+ skip = true;
}
+ if (skip)
+ continue;
if (rdev->raid_disk < 0)
seq_printf(seq, "(S)"); /* spare */
if (test_bit(Replacement, &rdev->flags))
--
2.4.6
^ permalink raw reply related
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