* Re: raid5-cache I/O path improvements V2
From: Christoph Hellwig @ 2015-09-17 1:53 UTC (permalink / raw)
To: Shaohua Li
Cc: Neil Brown, Christoph Hellwig, linux-raid@vger.kernel.org,
Kernel Team, dan.j.williams@intel.com
In-Reply-To: <20150915215458.GA1943628@devbig084.prn1.facebook.com>
On Tue, Sep 15, 2015 at 02:54:59PM -0700, Shaohua Li wrote:
> > I don't want a sysfs config if we can possibly avoid it.
> >
> > Christoph's code sets FUA on every block written to the log, both data
> > and metadata. Is that really what we want?
> >
> > I don't know much of the hardware details, but wouldn't setting FUA and
> > FLUSH on the last block written be just as effective and possibly faster
> > (by giving more flexibility to lower layers)??
>
> How is it different against without FUA, eg, doing a flush after several bios?
It's just a more complicated version of doing the same..
^ permalink raw reply
* Re: [PATCH 02/12] raid5-cache: free I/O units earlier
From: Christoph Hellwig @ 2015-09-17 1:50 UTC (permalink / raw)
To: Neil Brown; +Cc: Shaohua Li, linux-raid, Kernel-team, dan.j.williams
In-Reply-To: <87d1xk6ub4.fsf@notabene.neil.brown.name>
On Tue, Sep 15, 2015 at 09:00:31AM +0200, Neil Brown wrote:
> This only saves the allocation for two I/O units doesn't it? So not a
> big saveding, but still a good clean-up.
Yes.
> > r5l_do_reclaim as a side effect: previous if took the last unit which
> > isn't checkpointed into account.
>
> It took me a while to see that - so just to be sure I understand.
>
> There is a list of I/O units which have been completely written to the
> log and to the RAID. The last one of these is used as a marker for the
> start of the log, so it cannot be freed yet. All the earlier ones can
> be freed.
> The previous code added up the sizes of all of the units in this list,
> so it incorrectly included that last unit.
> Your new code calculates the difference between the start of the first
> unit and the start of the last unit, and so correctly excluded the last
> unit from the free space calculation.
>
> Did I get that right?
You did - or at very least your understanding matches my understanding of
the old code.
> > + md_wakeup_thread(log->rdev->mddev->thread);
> > + wait_event_lock_irq(log->iounit_wait,
> > + r5l_reclaimable_space(log) > reclaimable,
> > + log->io_list_lock);
> > }
>
> This is ringing warning bells.... I'm not sure it's wrong, but I'm not
> sure it is right.
> I feel that if the thread gets woken and finds that
> r5l_reclaimable_space is still too low, it should wake up the thread
> again, just in case.
> Also, it should test against reclaim_target, not reclaimable, shouldn't
> it?
>
> Instead of a wait_event_lock_irq inside a loop, could we just have a
> wake_event_*??
>
> wait_event_lock_irq_cmd(log->iounit_wait,
> r5l_reclaimable_space(log) >= reclaim_target ||
> (list_empty() && .... list_empty()),
> log->io_list_lock,
> md_wakeup_thread(log->rdev->mddev->thread));
>
> ??
>
>
> Otherwise I like it. Thanks.
I agree with a lot of your points, but I tried to match the old behavior
as close as possible. I can change it to something closer to your
suggestion if Shaohua agrees.
^ permalink raw reply
* Re: [PATCH 02/12] raid5-cache: free I/O units earlier
From: Christoph Hellwig @ 2015-09-17 1:48 UTC (permalink / raw)
To: Neil Brown; +Cc: Shaohua Li, linux-raid, Kernel-team, dan.j.williams
In-Reply-To: <87zj0o5cnj.fsf@notabene.neil.brown.name>
On Tue, Sep 15, 2015 at 10:07:12AM +0200, Neil Brown wrote:
> Christoph Hellwig <hch@lst.de> writes:
>
> > + sector_t reclaimable;
>
> sector_t is unsigned (u64 or "unsigned long"), so
>
> > + BUG_ON(reclaimable < 0);
>
> This could only be a compiler bug.
Indeed, I'll fix this up.
^ permalink raw reply
* Fwd: mdadm: not rebuilding IMSM array without OROM
From: Justin Capella @ 2015-09-16 17:54 UTC (permalink / raw)
To: linux-raid
In-Reply-To: <CAMrEMU_LmmB-fmLkW1g6ptYnM-qbXNBJL58NR0Ok8Ecx_16KRA@mail.gmail.com>
In reference to the following commit:
http://git.neil.brown.name/?p=mdadm.git;a=commit;h=7eee461e91df702d8f588955b03e5bbb07e1f88d
I am unclear how to proceed given my unfortunate environment, where I
have replaced the motherboard. I am fearful of reconfiguring the raid
in OROM in case it causes data loss. Is anyone aware of a tool to
rebuild/configure the OROM settings from the autodetected ones?
Should I just modify my startup scripts in my initrd by hand to include --force?
^ permalink raw reply
* Re: Recommendations for RAID setup needed
From: Alex @ 2015-09-16 15:53 UTC (permalink / raw)
To: Adam Goryachev; +Cc: Wols Lists, Linux RAID
In-Reply-To: <55F8D01C.5040106@websitemanagers.com.au>
Hi,
>>> What disks are you using? Are they proper raid disks? A 12TB array can
>>> have a soft read error every complete pass, and still be within the
>>> disk-manufacturer's specs. If your disks are not raid-compliant, this
>>> will stop your array from rebuilding, ever!
>>
>> All four are WD30EFRX-68EUZN0. They're not the cheapest WD disks, but
>> they're also not the ones with the 5yr warranty. The last array I
>> built using disks with 5yr warranty exceeded their capacity before the
>> warranty expired.
>
> Umm, 1st google result showed this:
> http://community.wd.com/t5/Desktop-Mobile-Drives/New-WD30EFRX-Red-Drive-Idle3-Timer-Set-to-8-Seconds-High-LCC-in/td-p/648821/page/5
I was really just looking for general input on RAID5 vs RAID6, but
that is good information. I knew the drives weren't basic desktop
drives and would be generally suitable for building a software RAID
array.
Are you familiar with the idle3 time? It appears the idle3-tools can
be used to disable the idle3 timer entirely, which would disable
parking the head at all, correct?
> You might want to verify that setting before using in production, and
> probably a quick search/read on any other issues.
Perhaps I should have posted here prior to ordering the drives. Do you
have any recommendations for 3TB SATA disks I should have used?
> That depends on your requirements. What are the implications (for you) if
> all the data is lost because two drives failed close to the same time? Is
> that resulting cost more or less than getting a fourth drive and using
> RAID6?
I was hoping for some kind of emphatic NO, that it's a really bad
idea. I'll consider it further, but probably choose RAID6 then anyway.
> Take a look at BackupPC (on sourceforge). It is a "perl script" that uses
> rsync plus hardlinks, but also does a whole lot more. It has worked very
> well for me for a number of years.
Okay, great.
> Personally, I'm also still waiting for a more "stable" version of btrfs or
> equivalent which can do block level de-dupe.
Awesome, thanks.
Alex
^ permalink raw reply
* [PATCH 1/1] md/raid1: Avoid raid1 resync getting stuck
From: Jes.Sorensen @ 2015-09-16 14:20 UTC (permalink / raw)
To: neilb; +Cc: linux-raid, nate.dailey
In-Reply-To: <1442413205-9593-1-git-send-email-Jes.Sorensen@redhat.com>
From: Jes Sorensen <Jes.Sorensen@redhat.com>
close_sync() needs to set conf->next_resync to a large, but safe value
below MaxSector and use it to determine whether or not to set
start_next_window in wait_barrier()
Solution suggested by Neil Brown.
Reported-by: Nate Dailey <nate.dailey@stratus.com>
Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
drivers/md/raid1.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 4517f06..763a0a8 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -881,8 +881,7 @@ static sector_t wait_barrier(struct r1conf *conf, struct bio *bio)
}
if (bio && bio_data_dir(bio) == WRITE) {
- if (bio->bi_iter.bi_sector >=
- conf->mddev->curr_resync_completed) {
+ if (bio->bi_iter.bi_sector >= conf->next_resync) {
if (conf->start_next_window == MaxSector)
conf->start_next_window =
conf->next_resync +
@@ -1516,7 +1515,7 @@ static void close_sync(struct r1conf *conf)
conf->r1buf_pool = NULL;
spin_lock_irq(&conf->resync_lock);
- conf->next_resync = 0;
+ conf->next_resync = MaxSector - 2 * NEXT_NORMALIO_DISTANCE;
conf->start_next_window = MaxSector;
conf->current_window_requests +=
conf->next_window_requests;
--
2.4.3
^ permalink raw reply related
* [PATCH 0/1] Fix raid1 resync getting stuck
From: Jes.Sorensen @ 2015-09-16 14:20 UTC (permalink / raw)
To: neilb; +Cc: linux-raid, nate.dailey
From: Jes Sorensen <Jes.Sorensen@redhat.com>
Neil,
This patch seems to do the job for us. Nate ran testing with > 500
iterations which would normally fail in less than 10.
I tried changing MaxSector to ((~(sector_t)0) >> 1) as you suggested,
but that made the raid5 code go explosive. This on the other hand
seems to do the trick.
Cheers,
Jes
Jes Sorensen (1):
md/raid1: Avoid raid1 resync getting stuck
drivers/md/raid1.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
--
2.4.3
^ permalink raw reply
* Re: Recommendations for RAID setup needed
From: Jens-U. Mozdzen @ 2015-09-16 10:03 UTC (permalink / raw)
Cc: Linux RAID
In-Reply-To: <55F8D01C.5040106@websitemanagers.com.au>
Hi Alex & *,
Zitat von Adam Goryachev <mailinglists@websitemanagers.com.au>:
> On 16/09/15 04:44, Alex wrote:
>> Hi,
>>
>>>> [...] but I'm
>>>> not sure I want to lose the extra space with creating a RAID6 array. I
>>>> believe RAID5 also has faster write speeds?
>>> What disks are you using? Are they proper raid disks? A 12TB array can
>>> have a soft read error every complete pass, and still be within the
>>> disk-manufacturer's specs. If your disks are not raid-compliant, this
>>> will stop your array from rebuilding, ever!
>> All four are WD30EFRX-68EUZN0. They're not the cheapest WD disks, but
>> they're also not the ones with the 5yr warranty. The last array I
>> built using disks with 5yr warranty exceeded their capacity before the
>> warranty expired.
> [...]
>>> (Chances are, your disks are above spec and won't give a problem. Do you
>>> want to take the risk?)
>> There's always going to be some kind of risk, but I'm hoping someone
>> with the technical understanding about disk failure rates can tell me
>> if it's a prudent decision or not.
> That depends on your requirements. What are the implications (for
> you) if all the data is lost because two drives failed close to the
> same time? Is that resulting cost more or less than getting a fourth
> drive and using RAID6?
I just want to point out that double and triple disk faults are far
beyond theory - we've had a RAID6 crash last year, where three of 11
disks failed within 24h. Two were from one batch, the third from a
different batch.
Alex, you initially said this is to be a backup server - so if that
means the data is already redundant, then you may stick with RAID5.
But if this is your main data storage and you rely on that data being
available (i.e. quicker than restoring from backup will take), go for
RAID6 and keep in mind that even that won't prevent you from total
RAID loss.
Regards,
Jens
^ permalink raw reply
* Re: Recommendations for RAID setup needed
From: Adam Goryachev @ 2015-09-16 2:12 UTC (permalink / raw)
To: Alex, Wols Lists; +Cc: Linux RAID
In-Reply-To: <CAB1R3sj3fA3rJA+hQRJcTvL6HmNium+T2RRfdvF9YhpLA=q2Vg@mail.gmail.com>
On 16/09/15 04:44, Alex wrote:
> Hi,
>
>>> I have a fedora22 system and would like to build a backup server. I
>>> have four 3TB SATA disks and would like to build a RAID5 array. I
>>> understand rebuild times can be extensive, possibility creating a
>>> scenario where another disk fails during that rebuild time, but I'm
>>> not sure I want to lose the extra space with creating a RAID6 array. I
>>> believe RAID5 also has faster write speeds?
>> What disks are you using? Are they proper raid disks? A 12TB array can
>> have a soft read error every complete pass, and still be within the
>> disk-manufacturer's specs. If your disks are not raid-compliant, this
>> will stop your array from rebuilding, ever!
> All four are WD30EFRX-68EUZN0. They're not the cheapest WD disks, but
> they're also not the ones with the 5yr warranty. The last array I
> built using disks with 5yr warranty exceeded their capacity before the
> warranty expired.
Umm, 1st google result showed this:
http://community.wd.com/t5/Desktop-Mobile-Drives/New-WD30EFRX-Red-Drive-Idle3-Timer-Set-to-8-Seconds-High-LCC-in/td-p/648821/page/5
You might want to verify that setting before using in production, and
probably a quick search/read on any other issues.
>> (Chances are, your disks are above spec and won't give a problem. Do you
>> want to take the risk?)
> There's always going to be some kind of risk, but I'm hoping someone
> with the technical understanding about disk failure rates can tell me
> if it's a prudent decision or not.
That depends on your requirements. What are the implications (for you)
if all the data is lost because two drives failed close to the same
time? Is that resulting cost more or less than getting a fourth drive
and using RAID6?
>>> Is a 9TB RAID5 partition too risky in terms of rebuild time?
>>>
>>> What's the preferred filesystem for a backup server these days? Should
>>> I use XFS or ext4?
>> Throwing something completely different into the mix, how about
>> considering btrfs? It's not 100% solid yet, so you need to be careful
>> with it, but if you back up with rsync and the "in place" option, it'll
>> give you full backups for the cost of incremental.
> I'm not sure I'm ready for something so experimental.
>
> I am in fact using the hard-link function of rsync to perform backups,
> though. We have a pretty robust perl script that's evolved over time.
> I was also thinking of implementing bacula, but not sure I have the
> time to figure it out right now.
Take a look at BackupPC (on sourceforge). It is a "perl script" that
uses rsync plus hardlinks, but also does a whole lot more. It has worked
very well for me for a number of years.
Personally, I'm also still waiting for a more "stable" version of btrfs
or equivalent which can do block level de-dupe.
Regards,
Adam
--
Adam Goryachev Website Managers www.websitemanagers.com.au
^ permalink raw reply
* Re: raid5-cache I/O path improvements V2
From: Shaohua Li @ 2015-09-15 21:54 UTC (permalink / raw)
To: Neil Brown
Cc: Christoph Hellwig, linux-raid@vger.kernel.org, Kernel Team,
dan.j.williams@intel.com
In-Reply-To: <87a8so6t8f.fsf@notabene.neil.brown.name>
On Tue, Sep 15, 2015 at 09:23:44AM +0200, Neil Brown wrote:
> Shaohua Li <shli@fb.com> writes:
>
> > On 9/11/15, 11:17 PM, "Christoph Hellwig" <hch@lst.de> wrote:
> >
> >>Hi Shaohua, hi Neil,
> >>
> >>this series contains a few updates to the raid5-cache feature.
> >>
> >>The first patch just ports it to the post-4.2 block layer. As part of
> >>that
> >>I noticed that it currently doesn't handle I/O errors - fixes for that
> >>will
> >>follow.
> >>
> >>The second and third patch simplify the I/O unit state machine and reduce
> >>latency and memory usage for the I/O units. The remainder are just a
> >>couple
> >>of cleanups in this area that I stumbled upon.
> >>
> >>Changes since V1:
> >> - only use REQ_FUA if supported natively by the log device
> >
> > Hi Christoph,
> >
> > I finally got some data with a Samsung SSD, which supports fua. Controller
> > is ahci.
> > Test is a simple fio with all full stripe write.
> >
> > libata.fua=0, throughput 247m/s
> > libata.fua=1, throughput 74m/s
>
> Eek! That's a big price to pay!
>
> >
> > fua is significantly slower. I think we need a sysfs config to enable fua.
>
> I don't want a sysfs config if we can possibly avoid it.
>
> Christoph's code sets FUA on every block written to the log, both data
> and metadata. Is that really what we want?
>
> I don't know much of the hardware details, but wouldn't setting FUA and
> FLUSH on the last block written be just as effective and possibly faster
> (by giving more flexibility to lower layers)??
How is it different against without FUA, eg, doing a flush after several bios?
Thanks,
Shaohua
^ permalink raw reply
* Re: Recommendations for RAID setup needed
From: Roman Mamedov @ 2015-09-15 18:57 UTC (permalink / raw)
To: Wols Lists; +Cc: Alex, Linux RAID
In-Reply-To: <55F85E9F.1070100@youngman.org.uk>
[-- Attachment #1: Type: text/plain, Size: 1115 bytes --]
On Tue, 15 Sep 2015 19:08:31 +0100
Wols Lists <antlists@youngman.org.uk> wrote:
> Throwing something completely different into the mix, how about
> considering btrfs? It's not 100% solid yet, so you need to be careful
> with it, but if you back up with rsync and the "in place" option, it'll
> give you full backups for the cost of incremental.
However its RAID5/6 is not ready yet and even RAID1/10 lack some important
features compared to mdadm, not to mention performance optimizations.
On the other hand I have a great success running Btrfs without utilizing its
own RAID features, but as a regular filesystem on top of MD RAID5/6.
> What you MUST do is KEEP AN EYE ON DISK SPACE! The main failure mode for
> btrfs I'm aware of, is that a disk full can cause a fatal error. As in
> "I've just trashed the disk - it's 'format c:' time". So if you hit 80%
> or so, alarm bells should be ringing. Very loud.
And this is just an absolute deranged baseless FUD today, or maybe something
that might have been true about 5 years ago -- eons in Btrfs development.
--
With respect,
Roman
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: Recommendations for RAID setup needed
From: Alex @ 2015-09-15 18:44 UTC (permalink / raw)
To: Wols Lists; +Cc: Linux RAID
In-Reply-To: <55F85E9F.1070100@youngman.org.uk>
Hi,
>> I have a fedora22 system and would like to build a backup server. I
>> have four 3TB SATA disks and would like to build a RAID5 array. I
>> understand rebuild times can be extensive, possibility creating a
>> scenario where another disk fails during that rebuild time, but I'm
>> not sure I want to lose the extra space with creating a RAID6 array. I
>> believe RAID5 also has faster write speeds?
>
> What disks are you using? Are they proper raid disks? A 12TB array can
> have a soft read error every complete pass, and still be within the
> disk-manufacturer's specs. If your disks are not raid-compliant, this
> will stop your array from rebuilding, ever!
All four are WD30EFRX-68EUZN0. They're not the cheapest WD disks, but
they're also not the ones with the 5yr warranty. The last array I
built using disks with 5yr warranty exceeded their capacity before the
warranty expired.
> (Chances are, your disks are above spec and won't give a problem. Do you
> want to take the risk?)
There's always going to be some kind of risk, but I'm hoping someone
with the technical understanding about disk failure rates can tell me
if it's a prudent decision or not.
>> Is a 9TB RAID5 partition too risky in terms of rebuild time?
>>
>> What's the preferred filesystem for a backup server these days? Should
>> I use XFS or ext4?
>
> Throwing something completely different into the mix, how about
> considering btrfs? It's not 100% solid yet, so you need to be careful
> with it, but if you back up with rsync and the "in place" option, it'll
> give you full backups for the cost of incremental.
I'm not sure I'm ready for something so experimental.
I am in fact using the hard-link function of rsync to perform backups,
though. We have a pretty robust perl script that's evolved over time.
I was also thinking of implementing bacula, but not sure I have the
time to figure it out right now.
Thanks,
Alex
^ permalink raw reply
* Re: dm: ioctl: prevent double freeing
From: Mike Snitzer @ 2015-09-15 18:13 UTC (permalink / raw)
To: Darrick J. Wong
Cc: device-mapper development, linux-kernel, Neil Brown, linux-raid,
Sudip Mukherjee, Alasdair Kergon
In-Reply-To: <20150915161855.GE10399@birch.djwong.org>
On Tue, Sep 15 2015 at 12:18pm -0400,
Darrick J. Wong <darrick.wong@oracle.com> wrote:
> On Fri, Sep 04, 2015 at 04:52:39PM +0530, Sudip Mukherjee wrote:
> > DM_PARAMS_KMALLOC and DM_PARAMS_VMALLOC should never be set together in
> > param_flags. We are setting these flags while allocating so we know that
> > there is almost no chance of having these two set together but still we
> > can have some additional safety.
> >
> > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> > ---
> > drivers/md/dm-ioctl.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> > index 80a4395..aaad74e 100644
> > --- a/drivers/md/dm-ioctl.c
> > +++ b/drivers/md/dm-ioctl.c
> > @@ -1680,7 +1680,7 @@ static void free_params(struct dm_ioctl *param, size_t param_size, int param_fla
> >
> > if (param_flags & DM_PARAMS_KMALLOC)
> > kfree(param);
> > - if (param_flags & DM_PARAMS_VMALLOC)
> > + else if (param_flags & DM_PARAMS_VMALLOC)
>
> Wouldn't you also want a WARN_ON to complain about both flags being set?
No, this is getting rediculous now. Please stop the insanity.
DM_PARAMS_KMALLOC and DM_PARAMS_VMALLOC are mutually exclussive --
always will be. Sprinkling defensive code around isn't high on my list
of changes I'm interested in.
At some point I'll stage the original change from Sudip for 4.4 but I'm
re-writing the subject and header to not be so alarmist (this is more to
do with eliminating a conditional than anything else). There is _zero_
potential for a double-free with the current code.
Mike
^ permalink raw reply
* Re: Recommendations for RAID setup needed
From: Wols Lists @ 2015-09-15 18:08 UTC (permalink / raw)
To: Alex, Linux RAID
In-Reply-To: <CAB1R3sjKb-aXJV+4E5t6OsWid4QB+r6aCteZt+F+57=cWLhHWw@mail.gmail.com>
On 15/09/15 18:39, Alex wrote:
> Hi,
> I have a fedora22 system and would like to build a backup server. I
> have four 3TB SATA disks and would like to build a RAID5 array. I
> understand rebuild times can be extensive, possibility creating a
> scenario where another disk fails during that rebuild time, but I'm
> not sure I want to lose the extra space with creating a RAID6 array. I
> believe RAID5 also has faster write speeds?
What disks are you using? Are they proper raid disks? A 12TB array can
have a soft read error every complete pass, and still be within the
disk-manufacturer's specs. If your disks are not raid-compliant, this
will stop your array from rebuilding, ever!
(Chances are, your disks are above spec and won't give a problem. Do you
want to take the risk?)
>
> Is a 9TB RAID5 partition too risky in terms of rebuild time?
>
> What's the preferred filesystem for a backup server these days? Should
> I use XFS or ext4?
Throwing something completely different into the mix, how about
considering btrfs? It's not 100% solid yet, so you need to be careful
with it, but if you back up with rsync and the "in place" option, it'll
give you full backups for the cost of incremental.
What you MUST do is KEEP AN EYE ON DISK SPACE! The main failure mode for
btrfs I'm aware of, is that a disk full can cause a fatal error. As in
"I've just trashed the disk - it's 'format c:' time". So if you hit 80%
or so, alarm bells should be ringing. Very loud.
>
> Thanks,
> Alex
Cheers,
Wol
^ permalink raw reply
* Recommendations for RAID setup needed
From: Alex @ 2015-09-15 17:39 UTC (permalink / raw)
To: Linux RAID
Hi,
I have a fedora22 system and would like to build a backup server. I
have four 3TB SATA disks and would like to build a RAID5 array. I
understand rebuild times can be extensive, possibility creating a
scenario where another disk fails during that rebuild time, but I'm
not sure I want to lose the extra space with creating a RAID6 array. I
believe RAID5 also has faster write speeds?
Is a 9TB RAID5 partition too risky in terms of rebuild time?
What's the preferred filesystem for a backup server these days? Should
I use XFS or ext4?
Thanks,
Alex
^ permalink raw reply
* Re: [dm-devel] [PATCH] dm: ioctl: prevent double freeing
From: Darrick J. Wong @ 2015-09-15 16:18 UTC (permalink / raw)
To: device-mapper development
Cc: Alasdair Kergon, Mike Snitzer, Neil Brown, linux-raid,
linux-kernel, Sudip Mukherjee
In-Reply-To: <1441365759-7247-1-git-send-email-sudipm.mukherjee@gmail.com>
On Fri, Sep 04, 2015 at 04:52:39PM +0530, Sudip Mukherjee wrote:
> DM_PARAMS_KMALLOC and DM_PARAMS_VMALLOC should never be set together in
> param_flags. We are setting these flags while allocating so we know that
> there is almost no chance of having these two set together but still we
> can have some additional safety.
>
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
> drivers/md/dm-ioctl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> index 80a4395..aaad74e 100644
> --- a/drivers/md/dm-ioctl.c
> +++ b/drivers/md/dm-ioctl.c
> @@ -1680,7 +1680,7 @@ static void free_params(struct dm_ioctl *param, size_t param_size, int param_fla
>
> if (param_flags & DM_PARAMS_KMALLOC)
> kfree(param);
> - if (param_flags & DM_PARAMS_VMALLOC)
> + else if (param_flags & DM_PARAMS_VMALLOC)
Wouldn't you also want a WARN_ON to complain about both flags being set?
--D
> vfree(param);
> }
>
> --
> 1.9.1
>
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply
* Re: raid1 resync stuck
From: Jes Sorensen @ 2015-09-15 15:25 UTC (permalink / raw)
To: Neil Brown; +Cc: majianpeng, linux-raid, nate.dailey
In-Reply-To: <8737yg6rbl.fsf@notabene.neil.brown.name>
Neil Brown <neilb@suse.de> writes:
> Jes Sorensen <Jes.Sorensen@redhat.com> writes:
>
>>> crash> r1conf 0xffff882028f3e600 | grep -e array_frozen -e barrier -e start_next_window -e next_resync
>>> barrier = 0x1, (conf->barrier < RESYNC_DEPTH)
>>> array_frozen = 0x0, (!conf->array_frozen)
>>> next_resync = 0x3000,
>>> start_next_window = 0x3000,
>>>
>>> ie. next_resync == start_next_window, which will never wake up since
>>> start_next_window is smaller than next_resync + RESYNC_SECTORS.
>>>
>>> Have you seen anything like this?
>>
>> Looking further at this together with Nate. It looks like you had a
>> patch resolving something similar:
>
> I hope you realize that this a confirming-instance of my hypothesis that
> if I just ignore questions, the asker will eventually solve it
> themselves? Maybe I should just wait a bit longer...
Argh I screwed up again! :)
>> It looks to us like close_sync()'s conf->start_next_window = MaxSector
>> results in wait_barrier() triggering this when the outstanding IO
>> completes:
>>
>> if (bio && bio_data_dir(bio) == WRITE) {
>> if (bio->bi_sector >=
>> conf->mddev->curr_resync_completed) {
>> if (conf->start_next_window == MaxSector)
>> conf->start_next_window =
>> conf->next_resync +
>> NEXT_NORMALIO_DISTANCE;
>>
>> putting us into the situation where raise_barrier()'s condition never
>> completes:
>>
>> wait_event_lock_irq(conf->wait_barrier,
>> !conf->array_frozen &&
>> conf->barrier < RESYNC_DEPTH &&
>> conf->current_window_requests == 0 &&
>> (conf->start_next_window >=
>> conf->next_resync + RESYNC_SECTORS),
>> conf->resync_lock);
>>
>> So the question is, is it wrong for close_sync() to be setting
>> conf->start_next_window = MaxSector in the first place, or should it
>> only be doing this once all outstanding I/O has completed?
>
> I think it is right to set start_next_window = MaxSector, but I think it
> is wrong to set ->next_resync = 0;
>
> I think:
> close_sync() should set next_sync to some impossibly big number,
> but not quite MaxSector as we sometimes add RESYNC_SECTORS or
> NEXT_NORMALIO_DISTANCE.
> May mddev->resync_max_sectors would be sensible. Then raid1_resize
> would need to update it though.
> Or maybe we should make MaxSector a bit smaller so it is safe to
> add to it. ((~(sector_t)0)>>1) ??
>
> wait_barrier() should include ->next_resync in its decision about
> setting start_next_window. May just replace
> "mddev->curr_resync_completed" with "next_resync".
>
> Can you try that? Does it make sense to you too?
I think this makes sense - I'll spin a patch for it and see how it
works out.
Cheers,
Jes
^ permalink raw reply
* Re: [PATCH 02/12] raid5-cache: free I/O units earlier
From: Neil Brown @ 2015-09-15 8:07 UTC (permalink / raw)
To: Christoph Hellwig, Shaohua Li; +Cc: linux-raid, Kernel-team, dan.j.williams
In-Reply-To: <1442038638-6947-3-git-send-email-hch@lst.de>
[-- Attachment #1: Type: text/plain, Size: 194 bytes --]
Christoph Hellwig <hch@lst.de> writes:
> + sector_t reclaimable;
sector_t is unsigned (u64 or "unsigned long"), so
> + BUG_ON(reclaimable < 0);
This could only be a compiler bug.
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: raid1 resync stuck
From: Neil Brown @ 2015-09-15 8:05 UTC (permalink / raw)
To: Jes Sorensen; +Cc: majianpeng, linux-raid, nate.dailey
In-Reply-To: <wrfjsi6k4zqy.fsf@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 6024 bytes --]
Jes Sorensen <Jes.Sorensen@redhat.com> writes:
> Jes Sorensen <Jes.Sorensen@redhat.com> writes:
>> Neil,
>>
>> We're chasing a case where the raid1 code gets stuck during resync. Nate
>> is able to reproduce it much more reliably than me - so attaching his
>> reproducing script. Basically run it on an existing raid1 with internal
>> bitmap on rotating disk.
>>
>> Nate was able to bisect it to 79ef3a8aa1cb1523cc231c9a90a278333c21f761,
>> the original iobarrier rewrite patch, and it can be reproduced in
>> current Linus' top of trunk a794b4f3292160bb3fd0f1f90ec8df454e3b17b3.
>>
>> In Nate's analysis it hangs in raise_barrier():
>>
>> static void raise_barrier(struct r1conf *conf, sector_t sector_nr)
>> {
>> spin_lock_irq(&conf->resync_lock);
>>
>> /* Wait until no block IO is waiting */
>> wait_event_lock_irq(conf->wait_barrier, !conf->nr_waiting,
>> conf->resync_lock);
>>
>> /* block any new IO from starting */
>> conf->barrier++;
>> conf->next_resync = sector_nr;
>>
>> /* For these conditions we must wait:
>> * A: while the array is in frozen state
>> * B: while barrier >= RESYNC_DEPTH, meaning resync reach
>> * the max count which allowed.
>> * C: next_resync + RESYNC_SECTORS > start_next_window, meaning
>> * next resync will reach to the window which normal bios are
>> * handling.
>> * D: while there are any active requests in the current window.
>> */
>> wait_event_lock_irq(conf->wait_barrier,
>> !conf->array_frozen &&
>> conf->barrier < RESYNC_DEPTH &&
>> conf->current_window_requests == 0 &&
>> (conf->start_next_window >=
>> conf->next_resync + RESYNC_SECTORS),
>> conf->resync_lock);
>>
>> crash> r1conf 0xffff882028f3e600 | grep -e array_frozen -e barrier -e start_next_window -e next_resync
>> barrier = 0x1, (conf->barrier < RESYNC_DEPTH)
>> array_frozen = 0x0, (!conf->array_frozen)
>> next_resync = 0x3000,
>> start_next_window = 0x3000,
>>
>> ie. next_resync == start_next_window, which will never wake up since
>> start_next_window is smaller than next_resync + RESYNC_SECTORS.
>>
>> Have you seen anything like this?
>
> Looking further at this together with Nate. It looks like you had a
> patch resolving something similar:
I hope you realize that this a confirming-instance of my hypothesis that
if I just ignore questions, the asker will eventually solve it
themselves? Maybe I should just wait a bit longer...
>
> commit 669cc7ba77864e7b1ac39c9f2b2afb8730f341f4
> Author: NeilBrown <neilb@suse.de>
> Date: Thu Sep 4 16:30:38 2014 +1000
>
> md/raid1: clean up request counts properly in close_sync()
>
> If there are outstanding writes when close_sync is called,
> the change to ->start_next_window might cause them to
> decrement the wrong counter when they complete. Fix this
> by merging the two counters into the one that will be decremented.
>
> Having an incorrect value in a counter can cause raise_barrier()
> to hangs, so this is suitable for -stable.
>
> Fixes: 79ef3a8aa1cb1523cc231c9a90a278333c21f761
> cc: stable@vger.kernel.org (v3.13+)
> Signed-off-by: NeilBrown <neilb@suse.de>
>
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index ad0468c..a31c92b 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1545,8 +1545,13 @@ static void close_sync(struct r1conf *conf)
> mempool_destroy(conf->r1buf_pool);
> conf->r1buf_pool = NULL;
>
> + spin_lock_irq(&conf->resync_lock);
> conf->next_resync = 0;
> conf->start_next_window = MaxSector;
> + conf->current_window_requests +=
> + conf->next_window_requests;
> + conf->next_window_requests = 0;
> + spin_unlock_irq(&conf->resync_lock);
> }
>
> It looks to us like close_sync()'s conf->start_next_window = MaxSector
> results in wait_barrier() triggering this when the outstanding IO
> completes:
>
> if (bio && bio_data_dir(bio) == WRITE) {
> if (bio->bi_sector >=
> conf->mddev->curr_resync_completed) {
> if (conf->start_next_window == MaxSector)
> conf->start_next_window =
> conf->next_resync +
> NEXT_NORMALIO_DISTANCE;
>
> putting us into the situation where raise_barrier()'s condition never
> completes:
>
> wait_event_lock_irq(conf->wait_barrier,
> !conf->array_frozen &&
> conf->barrier < RESYNC_DEPTH &&
> conf->current_window_requests == 0 &&
> (conf->start_next_window >=
> conf->next_resync + RESYNC_SECTORS),
> conf->resync_lock);
>
> So the question is, is it wrong for close_sync() to be setting
> conf->start_next_window = MaxSector in the first place, or should it
> only be doing this once all outstanding I/O has completed?
I think it is right to set start_next_window = MaxSector, but I think it
is wrong to set ->next_resync = 0;
I think:
close_sync() should set next_sync to some impossibly big number,
but not quite MaxSector as we sometimes add RESYNC_SECTORS or
NEXT_NORMALIO_DISTANCE.
May mddev->resync_max_sectors would be sensible. Then raid1_resize
would need to update it though.
Or maybe we should make MaxSector a bit smaller so it is safe to
add to it. ((~(sector_t)0)>>1) ??
wait_barrier() should include ->next_resync in its decision about
setting start_next_window. May just replace
"mddev->curr_resync_completed" with "next_resync".
Can you try that? Does it make sense to you too?
>
> Nate tested a case where removing the MaxSector assignment from
> close_sync() but are there any side effects to doing that?
Probably. Not sure off hand what they are though. It certainly feels
untidy leaving start_next_window pointing in the middle of the array
when resync/recovery has stopped.
Thanks,
NeilBrown
>
> Cheers,
> Jes
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: [PATCH] dm: ioctl: prevent double freeing
From: Sudip Mukherjee @ 2015-09-15 7:53 UTC (permalink / raw)
To: Alasdair Kergon, Mike Snitzer, dm-devel, Neil Brown
Cc: linux-kernel, linux-raid
In-Reply-To: <1441365759-7247-1-git-send-email-sudipm.mukherjee@gmail.com>
On Fri, Sep 04, 2015 at 04:52:39PM +0530, Sudip Mukherjee wrote:
> DM_PARAMS_KMALLOC and DM_PARAMS_VMALLOC should never be set together in
> param_flags. We are setting these flags while allocating so we know that
> there is almost no chance of having these two set together but still we
> can have some additional safety.
>
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
A gentle ping.
regards
sudip
^ permalink raw reply
* Re: [PATCH 17/39] md: drop null test before destroy functions
From: Neil Brown @ 2015-09-15 7:29 UTC (permalink / raw)
To: Julia Lawall
Cc: sergey.senozhatsky, kernel-janitors, linux-raid, linux-kernel
In-Reply-To: <1442146532-9100-18-git-send-email-Julia.Lawall@lip6.fr>
[-- Attachment #1: Type: text/plain, Size: 3464 bytes --]
Julia Lawall <Julia.Lawall@lip6.fr> writes:
> Remove unneeded NULL test.
>
> The semantic patch that makes this change is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @@ expression x; @@
> -if (x != NULL)
> \(kmem_cache_destroy\|mempool_destroy\|dma_pool_destroy\)(x);
> // </smpl>
>
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>
> ---
> drivers/md/multipath.c | 3 +--
> drivers/md/raid1.c | 6 ++----
> drivers/md/raid10.c | 9 +++------
> drivers/md/raid5.c | 3 +--
> 4 files changed, 7 insertions(+), 14 deletions(-)
Applied, thanks.
NeilBrown
>
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 15ef2c6..09a12d7 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -2271,8 +2271,7 @@ static void shrink_stripes(struct r5conf *conf)
> drop_one_stripe(conf))
> ;
>
> - if (conf->slab_cache)
> - kmem_cache_destroy(conf->slab_cache);
> + kmem_cache_destroy(conf->slab_cache);
> conf->slab_cache = NULL;
> }
>
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 4517f06..5f4f553 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -2843,8 +2843,7 @@ static struct r1conf *setup_conf(struct mddev *mddev)
>
> abort:
> if (conf) {
> - if (conf->r1bio_pool)
> - mempool_destroy(conf->r1bio_pool);
> + mempool_destroy(conf->r1bio_pool);
> kfree(conf->mirrors);
> safe_put_page(conf->tmppage);
> kfree(conf->poolinfo);
> @@ -2946,8 +2945,7 @@ static void raid1_free(struct mddev *mddev, void *priv)
> {
> struct r1conf *conf = priv;
>
> - if (conf->r1bio_pool)
> - mempool_destroy(conf->r1bio_pool);
> + mempool_destroy(conf->r1bio_pool);
> kfree(conf->mirrors);
> safe_put_page(conf->tmppage);
> kfree(conf->poolinfo);
> diff --git a/drivers/md/multipath.c b/drivers/md/multipath.c
> index d222522..d132f06 100644
> --- a/drivers/md/multipath.c
> +++ b/drivers/md/multipath.c
> @@ -470,8 +470,7 @@ static int multipath_run (struct mddev *mddev)
> return 0;
>
> out_free_conf:
> - if (conf->pool)
> - mempool_destroy(conf->pool);
> + mempool_destroy(conf->pool);
> kfree(conf->multipaths);
> kfree(conf);
> mddev->private = NULL;
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 0fc33eb..7c99a40 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -3486,8 +3486,7 @@ static struct r10conf *setup_conf(struct mddev *mddev)
> printk(KERN_ERR "md/raid10:%s: couldn't allocate memory.\n",
> mdname(mddev));
> if (conf) {
> - if (conf->r10bio_pool)
> - mempool_destroy(conf->r10bio_pool);
> + mempool_destroy(conf->r10bio_pool);
> kfree(conf->mirrors);
> safe_put_page(conf->tmppage);
> kfree(conf);
> @@ -3682,8 +3681,7 @@ static int run(struct mddev *mddev)
>
> out_free_conf:
> md_unregister_thread(&mddev->thread);
> - if (conf->r10bio_pool)
> - mempool_destroy(conf->r10bio_pool);
> + mempool_destroy(conf->r10bio_pool);
> safe_put_page(conf->tmppage);
> kfree(conf->mirrors);
> kfree(conf);
> @@ -3696,8 +3694,7 @@ static void raid10_free(struct mddev *mddev, void *priv)
> {
> struct r10conf *conf = priv;
>
> - if (conf->r10bio_pool)
> - mempool_destroy(conf->r10bio_pool);
> + mempool_destroy(conf->r10bio_pool);
> safe_put_page(conf->tmppage);
> kfree(conf->mirrors);
> kfree(conf->mirrors_old);
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: raid5-cache I/O path improvements V2
From: Neil Brown @ 2015-09-15 7:23 UTC (permalink / raw)
To: Shaohua Li, Christoph Hellwig
Cc: linux-raid@vger.kernel.org, Kernel Team, dan.j.williams@intel.com
In-Reply-To: <D21C6812.74CB%shli@fb.com>
[-- Attachment #1: Type: text/plain, Size: 1378 bytes --]
Shaohua Li <shli@fb.com> writes:
> On 9/11/15, 11:17 PM, "Christoph Hellwig" <hch@lst.de> wrote:
>
>>Hi Shaohua, hi Neil,
>>
>>this series contains a few updates to the raid5-cache feature.
>>
>>The first patch just ports it to the post-4.2 block layer. As part of
>>that
>>I noticed that it currently doesn't handle I/O errors - fixes for that
>>will
>>follow.
>>
>>The second and third patch simplify the I/O unit state machine and reduce
>>latency and memory usage for the I/O units. The remainder are just a
>>couple
>>of cleanups in this area that I stumbled upon.
>>
>>Changes since V1:
>> - only use REQ_FUA if supported natively by the log device
>
> Hi Christoph,
>
> I finally got some data with a Samsung SSD, which supports fua. Controller
> is ahci.
> Test is a simple fio with all full stripe write.
>
> libata.fua=0, throughput 247m/s
> libata.fua=1, throughput 74m/s
Eek! That's a big price to pay!
>
> fua is significantly slower. I think we need a sysfs config to enable fua.
I don't want a sysfs config if we can possibly avoid it.
Christoph's code sets FUA on every block written to the log, both data
and metadata. Is that really what we want?
I don't know much of the hardware details, but wouldn't setting FUA and
FLUSH on the last block written be just as effective and possibly faster
(by giving more flexibility to lower layers)??
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: [PATCH 02/12] raid5-cache: free I/O units earlier
From: Neil Brown @ 2015-09-15 7:00 UTC (permalink / raw)
To: Christoph Hellwig, Shaohua Li; +Cc: linux-raid, Kernel-team, dan.j.williams
In-Reply-To: <1442038638-6947-3-git-send-email-hch@lst.de>
[-- Attachment #1: Type: text/plain, Size: 3721 bytes --]
Christoph Hellwig <hch@lst.de> writes:
> There is no good reason to keep the I/O unit structures around after the
> stripe has been written back to the RAID array. The only information
> we need is the log sequence number, and the checkpoint offset of the
> highest successfull writeback. Store those in the log structure, and
> free the IO units from __r5l_stripe_write_finished.
>
> Besides simplifying the code this also avoid having to keep the allocation
> for the I/O unit around for a potentially long time as superblock updates
> that checkpoint the log do not happen very often.
This only saves the allocation for two I/O units doesn't it? So not a
big saveding, but still a good clean-up.
>
> This also fixes the previously incorrect calculation of 'free' in
> r5l_do_reclaim as a side effect: previous if took the last unit which
> isn't checkpointed into account.
It took me a while to see that - so just to be sure I understand.
There is a list of I/O units which have been completely written to the
log and to the RAID. The last one of these is used as a marker for the
start of the log, so it cannot be freed yet. All the earlier ones can
be freed.
The previous code added up the sizes of all of the units in this list,
so it incorrectly included that last unit.
Your new code calculates the difference between the start of the first
unit and the start of the last unit, and so correctly excluded the last
unit from the free space calculation.
Did I get that right?
> @@ -690,60 +675,41 @@ static void r5l_do_reclaim(struct r5l_log *log)
> * shouldn't reuse space of an unreclaimable io_unit
> * */
> while (1) {
> - struct list_head *target_list = NULL;
> -
> - while (!list_empty(&log->stripe_end_ios)) {
> - io = list_first_entry(&log->stripe_end_ios,
> - struct r5l_io_unit, log_sibling);
> - list_move_tail(&io->log_sibling, &list);
> - free += r5l_ring_distance(log, io->log_start,
> - io->log_end);
> - }
> -
> - if (free >= reclaim_target ||
> + reclaimable = r5l_reclaimable_space(log);
> + if (reclaimable >= reclaim_target ||
> (list_empty(&log->running_ios) &&
> list_empty(&log->io_end_ios) &&
> list_empty(&log->flushing_ios) &&
> list_empty(&log->flushed_ios)))
> break;
>
> - /* Below waiting mostly happens when we shutdown the raid */
> - if (!list_empty(&log->flushed_ios))
> - target_list = &log->flushed_ios;
> - else if (!list_empty(&log->flushing_ios))
> - target_list = &log->flushing_ios;
> - else if (!list_empty(&log->io_end_ios))
> - target_list = &log->io_end_ios;
> - else if (!list_empty(&log->running_ios))
> - target_list = &log->running_ios;
> -
> - r5l_kick_io_unit(log);
> + md_wakeup_thread(log->rdev->mddev->thread);
> + wait_event_lock_irq(log->iounit_wait,
> + r5l_reclaimable_space(log) > reclaimable,
> + log->io_list_lock);
> }
This is ringing warning bells.... I'm not sure it's wrong, but I'm not
sure it is right.
I feel that if the thread gets woken and finds that
r5l_reclaimable_space is still too low, it should wake up the thread
again, just in case.
Also, it should test against reclaim_target, not reclaimable, shouldn't
it?
Instead of a wait_event_lock_irq inside a loop, could we just have a
wake_event_*??
wait_event_lock_irq_cmd(log->iounit_wait,
r5l_reclaimable_space(log) >= reclaim_target ||
(list_empty() && .... list_empty()),
log->io_list_lock,
md_wakeup_thread(log->rdev->mddev->thread));
??
Otherwise I like it. Thanks.
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: raid5-cache I/O path improvements V2
From: Shaohua Li @ 2015-09-14 19:11 UTC (permalink / raw)
To: Christoph Hellwig, neilb@suse.de
Cc: linux-raid@vger.kernel.org, Kernel Team, dan.j.williams@intel.com
In-Reply-To: <1442038638-6947-1-git-send-email-hch@lst.de>
On 9/11/15, 11:17 PM, "Christoph Hellwig" <hch@lst.de> wrote:
>Hi Shaohua, hi Neil,
>
>this series contains a few updates to the raid5-cache feature.
>
>The first patch just ports it to the post-4.2 block layer. As part of
>that
>I noticed that it currently doesn't handle I/O errors - fixes for that
>will
>follow.
>
>The second and third patch simplify the I/O unit state machine and reduce
>latency and memory usage for the I/O units. The remainder are just a
>couple
>of cleanups in this area that I stumbled upon.
>
>Changes since V1:
> - only use REQ_FUA if supported natively by the log device
Hi Christoph,
I finally got some data with a Samsung SSD, which supports fua. Controller
is ahci.
Test is a simple fio with all full stripe write.
libata.fua=0, throughput 247m/s
libata.fua=1, throughput 74m/s
fua is significantly slower. I think we need a sysfs config to enable fua.
Thanks,
Shaohua
^ permalink raw reply
* Re: [PATCH 12/39] dm: drop null test before destroy functions
From: Julia Lawall @ 2015-09-14 18:50 UTC (permalink / raw)
To: Mikulas Patocka
Cc: Mike Snitzer, device-mapper development, Alasdair Kergon,
kernel-janitors, Neil Brown, linux-kernel, linux-raid,
sergey.senozhatsky
In-Reply-To: <alpine.LRH.2.02.1509141004430.7970@file01.intranet.prod.int.rdu2.redhat.com>
On Mon, 14 Sep 2015, Mikulas Patocka wrote:
>
>
> On Mon, 14 Sep 2015, Mike Snitzer wrote:
>
> > On Mon, Sep 14 2015 at 9:46am -0400,
> > Mikulas Patocka <mpatocka@redhat.com> wrote:
> >
> > >
> > >
> > > On Sun, 13 Sep 2015, Julia Lawall wrote:
> > >
> > > > Remove unneeded NULL test.
> > > >
> > > > The semantic patch that makes this change is as follows:
> > > > (http://coccinelle.lip6.fr/)
> > > >
> > > > // <smpl>
> > > > @@ expression x; @@
> > > > -if (x != NULL)
> > > > \(kmem_cache_destroy\|mempool_destroy\|dma_pool_destroy\)(x);
> > > > // </smpl>
> > > >
> > > > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> > > >
> > > > ---
> > > > drivers/md/dm-bufio.c | 3 +--
> > > > drivers/md/dm-cache-target.c | 3 +--
> > > > drivers/md/dm-crypt.c | 6 ++----
> > > > drivers/md/dm-io.c | 3 +--
> > > > drivers/md/dm-log-userspace-base.c | 3 +--
> > > > drivers/md/dm-region-hash.c | 4 +---
> > > > drivers/md/dm.c | 13 ++++---------
> > > > 7 files changed, 11 insertions(+), 24 deletions(-)
> > > >
> > > > diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
> > > > index 83cc52e..8ad39b6 100644
> > > > --- a/drivers/md/dm-bufio.c
> > > > +++ b/drivers/md/dm-bufio.c
> > > > @@ -1864,8 +1864,7 @@ static void __exit dm_bufio_exit(void)
> > > > for (i = 0; i < ARRAY_SIZE(dm_bufio_caches); i++) {
> > > > struct kmem_cache *kc = dm_bufio_caches[i];
> > > >
> > > > - if (kc)
> > > > - kmem_cache_destroy(kc);
> > > > + kmem_cache_destroy(kc);
> > > > }
> > >
> > > The variable here can be NULL. I don't know how did you conclude that it
> > > cannot. It seems that you didn't test the patch, if you did, you'd hit
> > > NULL pointer dereference here.
> >
> > kmem_cache_destroy(), mempool_destroy(), etc all check for NULL and just
> > return. So there is no need for the callers to check for NULL too.
> >
> > Mike
>
> I see. It was recent change that I missed.
The relevant commits were in the cover letter:
kmem_cache_destroy (commit 3942d29918522)
mempool_destroy (commit 4e3ca3e033d1)
dma_pool_destroy (commit 44d7175da6ea)
julia
^ 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