* Re: md/dm-crypt: Rename a jump label in crypt_message() ?
From: SF Markus Elfring @ 2016-09-30 11:53 UTC (permalink / raw)
To: Bjørn Mork
Cc: Dan Carpenter, Theodore Ts'o, dm-devel, linux-raid,
Alasdair Kergon, Mike Snitzer, Shaohua Li, Julia Lawall,
kernel-janitors, LKML, linux-doc
In-Reply-To: <87mvip1us4.fsf@miraculix.mork.no>
> When someone tells you that you are wasting their time,
This information can be useful to some degree
> then that is not a subject for further discussion.
I got an other impression. I guess that there are constraints for such a response
which can become interesting for further considerations.
Is it just usual that other software changes are more welcome?
Regards,
Markus
^ permalink raw reply
* Re: md/dm-crypt: Rename a jump label in crypt_message() ?
From: Bjørn Mork @ 2016-09-30 11:39 UTC (permalink / raw)
To: SF Markus Elfring
Cc: Dan Carpenter, Theodore Ts'o, dm-devel, linux-raid,
Alasdair Kergon, Mike Snitzer, Shaohua Li, Julia Lawall,
kernel-janitors, LKML, linux-doc
In-Reply-To: <d9c6668b-65b3-e410-81ed-1e43c227f015@users.sourceforge.net>
SF Markus Elfring <elfring@users.sourceforge.net> writes:
>> go around bothering everyone with waste of time cleanup patches.
>
> I find it still debatable if the shown software development efforts
> are really "wasted".
When someone tells you that you are wasting their time, then that is not
a subject for further discussion.
Bjørn
^ permalink raw reply
* Re: md/dm-crypt: Rename a jump label in crypt_message() ?
From: SF Markus Elfring @ 2016-09-30 11:32 UTC (permalink / raw)
To: Dan Carpenter
Cc: Theodore Ts'o, dm-devel, linux-raid, Alasdair Kergon,
Mike Snitzer, Shaohua Li, Julia Lawall, kernel-janitors, LKML,
linux-doc
In-Reply-To: <20160930100603.GB13620@mwanda>
> Again, I wrote the paragraph in CodingStyle.
This is obvious from the corresponding commit "add some more error
handling guidelines" (ea04036032edda6f771c1381d03832d2ed0f6c31
on 2014-12-02).
> I just said that it's a good idea to think about label names
I agree also to such a desire.
> instead of using GW-BASIC style numbered labels,
Is this kind of wording another weakness in the discussed document?
How many guidance do programmers get from such a software specification?
I came a few source code places along where I proposed corresponding changes.
> I didn't say
You did not say anything about some details as it is often easier to express
several aspects in vague and general terms.
> go around bothering everyone with waste of time cleanup patches.
I find it still debatable if the shown software development efforts
are really "wasted".
It seems that also the Linux development community is mixed about
related interpretations.
> I specifically did not say that "out:" or "error:" labels are bad names.
Did you inform me once that you had also a special opinion about an identifier
like "out"?
The C compiler will accept them as usual. But do we occasionally prefer
to express implementation details a bit better there?
> Those are common style in the kernel.
* Which impressions can you get from a statement like "goto fail;"
or "goto error;"?
* Do any exception handling implementations should be reconsidered
at such places?
> Please stop sending these patches.
Could it happen that the change acceptance will increase also for
the suggested renaming of jump labels if maintainers from other subsystems
would dare to respond once more in a positive way for such a software refactoring?
Regards,
Markus
^ permalink raw reply
* Re: [dm-devel] [PATCH 03/10] md/dm-crypt: Rename a jump label in crypt_message()
From: Dan Carpenter @ 2016-09-30 10:06 UTC (permalink / raw)
To: SF Markus Elfring
Cc: Theodore Ts'o, dm-devel, linux-raid, Alasdair Kergon,
Mike Snitzer, Shaohua Li, Julia Lawall, kernel-janitors, LKML
In-Reply-To: <e40a8107-7400-bc10-a212-563dd168aac5@users.sourceforge.net>
On Thu, Sep 29, 2016 at 05:43:57PM +0200, SF Markus Elfring wrote:
> > In what bizzaro world is the "current Linux coding style convention"
>
> Do you look at the evolution for a document like "CodingStyle"?
>
Again, I wrote the paragraph in CodingStyle. I just said that it's a
good idea to think about label names instead of using GW-BASIC style
numbered labels, I didn't say go around bothering everyone with waste
of time cleanup patches.
I specifically did not say that "out:" or "error:" labels are bad names.
Those are common style in the kernel.
Please stop sending these patches.
regards,
dan carpenter
^ permalink raw reply
* Re: dm snapshot: Use kmalloc_array() in init_origin_hash() ?
From: SF Markus Elfring @ 2016-09-30 7:14 UTC (permalink / raw)
To: Paul Bolle
Cc: Joe Perches, Andy Whitcroft, dm-devel, linux-raid,
Alasdair Kergon, Mike Snitzer, Shaohua Li, LKML, kernel-janitors,
Julia Lawall
In-Reply-To: <1475185365.31297.50.camel@tiscali.nl>
> I've recently ping-ponged with the kernel's "resident wrong bot of the
> day" over this very rule (kmalloc_array() is safer than kmalloc(), so
> change your driver now!).
Your bot of the day is going to point more update candidates out
in various source files that can "accidentally" belong also to Linux. ;-)
> Could we just give wrong bots a bit less ammunition whenever that's feasible?
How do you think about to clarify constraints any further so that
the probability for false positives can be reduced as desired for
the involved source code analysis tools?
> Even if you don't care about my ping-pong experiences: this checkpatch
> test is broken, please just fix it!
I am curious how collateral software evolution will be continued.
Regards,
Markus
^ permalink raw reply
* Re: [PATCH v2 1/6] r5cache: write part of r5cache
From: Song Liu @ 2016-09-29 23:06 UTC (permalink / raw)
To: Shaohua Li
Cc: Song Liu, linux-raid@vger.kernel.org, neilb@suse.com, Shaohua Li,
Kernel Team, dan.j.williams@intel.com, hch@infradead.org,
liuzhengyuang521@gmail.com, liuzhengyuan@kylinos.cn
In-Reply-To: <20160927225120.GA98100@kernel.org>
Thanks Shaohua!
I am working on adding more information in comments and commit notes.
>>
>> +/*
>> + * Freeze the stripe, thus send the stripe into reclaim path.
>> + *
>> + * This function should only be called from raid5d that handling this stripe,
>> + * or when holds conf->device_lock
>> + */
>
> Do you mean if this called in raid5d, the lock isn't required? Please note we
> could have several threads (like raid5d) handle stripes. Is there any problem
> here?
This requirement is not necessary anymore. I will update the comment here.
>
>> +void r5c_freeze_stripe_for_reclaim(struct stripe_head *sh)
>> +{
>> + struct r5conf *conf = sh->raid_conf;
>> +
>> + if (!conf->log)
>> + return;
>> +static void r5c_handle_parity_cached(struct stripe_head *sh)
>> +{
>> + int i;
>> +
>> + for (i = sh->disks; i--; )
>> + if (test_bit(R5_InCache, &sh->dev[i].flags))
>> + set_bit(R5_Wantwrite, &sh->dev[i].flags);
>> + set_bit(STRIPE_R5C_WRITTEN, &sh->state);
>> +}
>> +
>> +static void r5c_finish_cache_stripe(struct stripe_head *sh)
>> +{
>
> I'm hoping this one has a
> if (not in writeback mode)
> return
> so it's clearly this is only for writeback mode.
>
>> + if (test_bit(STRIPE_R5C_FROZEN, &sh->state))
>> + r5c_handle_parity_cached(sh);
>> + else
>> + r5c_handle_data_cached(sh);
>> +}
>> +
>>
>> return 0;
>> }
>>
>> -static void r5l_wake_reclaim(struct r5l_log *log, sector_t space);
>> /*
>> * running in raid5d, where reclaim could wait for raid5d too (when it flushes
>> * data from log to raid disks), so we shouldn't wait for reclaim here
>> @@ -456,11 +541,17 @@ int r5l_write_stripe(struct r5l_log *log, struct stripe_head *sh)
>> return -EAGAIN;
>> }
>>
>> + WARN_ON(!test_bit(STRIPE_R5C_FROZEN, &sh->state));
>
> is this set even for write through?
In current implementation, the code still sets STRIPE_R5C_FROZEN in write through mode
(in r5c_handle_stripe_dirtying). The reclaim path handles has same behavior in write through
mode as original journal.
>>
>> +
>> + if (!log || test_bit(STRIPE_R5C_FROZEN, &sh->state))
>> + return -EAGAIN;
>> +
>> + if (conf->log->r5c_state == R5C_STATE_WRITE_THROUGH ||
>> + conf->quiesce != 0 || conf->mddev->degraded != 0) {
>> + /* write through mode */
>> + r5c_freeze_stripe_for_reclaim(sh);
>> + return -EAGAIN;
>
> will this change behavior of R5C_STATE_WRITE_THROUGH?
> r5c_freeze_stripe_for_reclaim does change something not related to writethrough
> mode.
The PREREAD_ACTIVE part of freeze is not necessary for write through. I will fix it.
Other that, the reclaim path works the same as write through mode (as original journal
code). I will double check and make it clear.
> The quiesce check sounds not necessary. The patch flush all caches in quiesce,
> so no IO is running in quiesce state.
Great catch. Other checks already covers all cases in quiesce.
>>
>> @@ -901,6 +918,13 @@ static void ops_run_io(struct stripe_head *sh, struct stripe_head_state *s)
>>
>> might_sleep();
>>
>> + if (s->to_cache) {
>> + if (r5c_cache_data(conf->log, sh, s) == 0)
>> + return;
>
> At this stage we fallback to no cache. But I don't see R5_Wantcache is cleared,
> is it a problem?
We did similar check for journal part. I think it will be easier if we add a check in
r5l_init_log(), and never create journal/cache when the array is too big?
>
>> @@ -1543,9 +1570,18 @@ ops_run_compute6_2(struct stripe_head *sh, struct raid5_percpu *percpu)
>> static void ops_complete_prexor(void *stripe_head_ref)
>> {
>> struct stripe_head *sh = stripe_head_ref;
>> + int i;
>>
>> pr_debug("%s: stripe %llu\n", __func__,
>> (unsigned long long)sh->sector);
>> +
>> + for (i = sh->disks; i--; )
>> + if (sh->dev[i].page != sh->dev[i].orig_page) {
>> + struct page *p = sh->dev[i].page;
>> +
>> + sh->dev[i].page = sh->dev[i].orig_page;
>> + put_page(p);
> What if array hasn't log but skipcopy is enabled?
In case of skip copy, page and orig_page are the same during ops_complete_prexor:
page == orig_page == old data.
bio_page is the new data.
After prexor is done, we run biodrain, where page is set to bio_page to skip copy.
>>
>> @@ -4416,7 +4538,7 @@ static void handle_stripe(struct stripe_head *sh)
>> struct r5dev *dev = &sh->dev[i];
>> if (test_bit(R5_LOCKED, &dev->flags) &&
>> (i == sh->pd_idx || i == sh->qd_idx ||
>> - dev->written)) {
>> + dev->written || test_bit(R5_InCache, &dev->flags))) {
>> pr_debug("Writing block %d\n", i);
>> set_bit(R5_Wantwrite, &dev->flags);
>> if (prexor)
>> @@ -4456,6 +4578,12 @@ static void handle_stripe(struct stripe_head *sh)
>> test_bit(R5_Discard, &qdev->flags))))))
>> handle_stripe_clean_event(conf, sh, disks, &s.return_bi);
>>
>> + if (s.just_cached)
>> + r5c_handle_cached_data_endio(conf, sh, disks, &s.return_bi);
>> +
>> + if (test_bit(STRIPE_R5C_FROZEN, &sh->state))
>> + r5l_stripe_write_finished(sh);
> what's this for?
r5l_stripe_write_finished() removes sh->log_io.
It is actually not necessary to check for R5C_FROZEN. I will fix that.
>
>> +
>> /* Now we might consider reading some blocks, either to check/generate
>> * parity, or to satisfy requests
>> * or to load a block that is being partially written.
>> @@ -4467,13 +4595,17 @@ static void handle_stripe(struct stripe_head *sh)
>> || s.expanding)
>> handle_stripe_fill(sh, &s, disks);
>>
>> - /* Now to consider new write requests and what else, if anything
>> - * should be read. We do not handle new writes when:
>> + r5c_handle_stripe_written(conf, sh);
>
> please explain why this is required?
Added the following in comments:
/*
* If the stripe finishes full journal write cycle (write to journal
* and raid disk, this is the clean up procedure so it is ready for
* next operation.
*/
>>
>> /* maybe we need to check and possibly fix the parity for this stripe
>> @@ -5192,7 +5324,7 @@ static void raid5_make_request(struct mddev *mddev, struct bio * bi)
>> * later we might have to read it again in order to reconstruct
>> * data on failed drives.
>> */
>> - if (rw == READ && mddev->degraded == 0 &&
>> + if (rw == READ && mddev->degraded == 0 && conf->log == NULL &&
>> mddev->reshape_position == MaxSector) {
>> bi = chunk_aligned_read(mddev, bi);
>> if (!bi)
> This should be only for R5C_STATE_WRITE_BACK.
Will fix this.
>>
>> @@ -7662,8 +7800,11 @@ static void raid5_quiesce(struct mddev *mddev, int state)
>> /* '2' tells resync/reshape to pause so that all
>> * active stripes can drain
>> */
>> + r5c_flush_cache(conf, 0);
>> conf->quiesce = 2;
>> wait_event_cmd(conf->wait_for_quiescent,
>> + atomic_read(&conf->r5c_cached_partial_stripes) == 0 &&
>> + atomic_read(&conf->r5c_cached_full_stripes) == 0 &&
> I don't see a wakeup for this after the check condition is met.
The following in release_inactive_stripe_list will wake this up.
if (atomic_read(&conf->active_stripes) == 0)
wake_up(&conf->wait_for_quiescent);
This is OK because cached stripes (full stripe or partial) has to become active
first before being inactive.
Thanks,
Song
^ permalink raw reply
* Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()
From: Paul Bolle @ 2016-09-29 21:42 UTC (permalink / raw)
To: Joe Perches, Andy Whitcroft
Cc: SF Markus Elfring, dm-devel, linux-raid, Alasdair Kergon,
Mike Snitzer, Shaohua Li, LKML, kernel-janitors, Julia Lawall
In-Reply-To: <1475184082.1954.5.camel@perches.com>
On Thu, 2016-09-29 at 14:21 -0700, Joe Perches wrote:
> > > It doesn't matter match either way to me.
> > Why does this stop you fixing an apparently wrong checkpatch rule,
> > crude as parts of it are (ie, uppercase identifier must be a
> > constant)?
>
> It doesn't. It just doesn't matter much (match) to me.
Joe, please.
I've recently ping-ponged with the kernel's "resident wrong bot of the
day" over this very rule (kmalloc_array() is safer than kmalloc(), so
change your driver now!). Could we just give wrong bots a bit less
ammunition whenever that's feasible?
Even if you don't care about my ping-pong experiences: this checkpatch
test is broken, please just fix it!
Paul Bolle
^ permalink raw reply
* Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()
From: Joe Perches @ 2016-09-29 21:21 UTC (permalink / raw)
To: Paul Bolle, Andy Whitcroft
Cc: SF Markus Elfring, dm-devel, linux-raid, Alasdair Kergon,
Mike Snitzer, Shaohua Li, LKML, kernel-janitors, Julia Lawall
In-Reply-To: <1475183693.31297.36.camel@tiscali.nl>
On Thu, 2016-09-29 at 23:14 +0200, Paul Bolle wrote:
> On Thu, 2016-09-29 at 13:56 -0700, Joe Perches wrote:
> > It doesn't matter match either way to me.
> Why does this stop you fixing an apparently wrong checkpatch rule,
> crude as parts of it are (ie, uppercase identifier must be a constant)?
It doesn't. It just doesn't matter much (match) to me.
Here:
---
scripts/checkpatch.pl | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 3373c65fef1c..fc931d89152e 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5835,8 +5835,8 @@ sub process {
if ($^V && $^V ge 5.10.0 &&
$line =~ /\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*(k[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)\s*,/) {
my $oldfunc = $3;
- my $a1 = $4;
- my $a2 = $10;
+ my $a1 = trim($4);
+ my $a2 = trim($10);
my $newfunc = "kmalloc_array";
$newfunc = "kcalloc" if ($oldfunc eq "kzalloc");
my $r1 = $a1;
@@ -5850,7 +5850,7 @@ sub process {
if (WARN("ALLOC_WITH_MULTIPLY",
"Prefer $newfunc over $oldfunc with multiply\n" . $herecurr) &&
$fix) {
- $fixed[$fixlinenr] =~ s/\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*(k[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)/$1 . ' = ' . "$newfunc(" . trim($r1) . ', ' . trim($r2)/e;
+ $fixed[$fixlinenr] =~ s/\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*(k[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)/$1 . ' = ' . "$newfunc(" . $r1 . ', ' . $r2/e;
}
}
^ permalink raw reply related
* Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()
From: Paul Bolle @ 2016-09-29 21:14 UTC (permalink / raw)
To: Joe Perches, Andy Whitcroft
Cc: SF Markus Elfring, dm-devel, linux-raid, Alasdair Kergon,
Mike Snitzer, Shaohua Li, LKML, kernel-janitors, Julia Lawall
In-Reply-To: <1475182586.1954.3.camel@perches.com>
On Thu, 2016-09-29 at 13:56 -0700, Joe Perches wrote:
> It doesn't matter match either way to me.
>
> The case for the unnecessary multiply with <= gcc 4.8 was
> removed with:
>
> commit 91c6a05f72a996bee5133e76374ab3ad7d3b9b72
> Author: Alexey Dobriyan <adobriyan@gmail.com>
> Date: Tue Jul 26 15:22:08 2016 -0700
>
> mm: faster kmalloc_array(), kcalloc()
>
> When both arguments to kmalloc_array() or kcalloc() are known at compile
> time then their product is known at compile time but search for kmalloc
> cache happens at runtime not at compile time.
>
> Link: http://lkml.kernel.org/r/20160627213454.GA2440@p183.telecom.by
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Pekka Enberg <penberg@kernel.org>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
You've lost me.
Why does this stop you fixing an apparently wrong checkpatch rule,
crude as parts of it are (ie, uppercase identifier must be a constant)?
Paul Bolle
^ permalink raw reply
* Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()
From: Joe Perches @ 2016-09-29 20:56 UTC (permalink / raw)
To: Paul Bolle, Andy Whitcroft
Cc: SF Markus Elfring, dm-devel, linux-raid, Alasdair Kergon,
Mike Snitzer, Shaohua Li, LKML, kernel-janitors, Julia Lawall
In-Reply-To: <1475181593.31297.25.camel@tiscali.nl>
On Thu, 2016-09-29 at 22:39 +0200, Paul Bolle wrote:
> On Thu, 2016-09-29 at 13:24 -0700, Joe Perches wrote:
> > On Thu, 2016-09-29 at 21:43 +0200, Paul Bolle wrote:
> > > Why doesn't that regex match on "ORIGIN_HASH_SIZE"?
> > It does match.
> If that regex does match, it being part of a negative test, the
> specific checkpatch rule should be silent, shouldn't it?
'cause I forgot to trim() the original $4 and $10 matches.
Oh well.
It doesn't matter match either way to me.
The case for the unnecessary multiply with <= gcc 4.8 was
removed with:
commit 91c6a05f72a996bee5133e76374ab3ad7d3b9b72
Author: Alexey Dobriyan <adobriyan@gmail.com>
Date: Tue Jul 26 15:22:08 2016 -0700
mm: faster kmalloc_array(), kcalloc()
When both arguments to kmalloc_array() or kcalloc() are known at compile
time then their product is known at compile time but search for kmalloc
cache happens at runtime not at compile time.
Link: http://lkml.kernel.org/r/20160627213454.GA2440@p183.telecom.by
Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Christoph Lameter <cl@linux.com>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: David Rientjes <rientjes@google.com>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
^ permalink raw reply
* Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()
From: Paul Bolle @ 2016-09-29 20:39 UTC (permalink / raw)
To: Joe Perches, Andy Whitcroft
Cc: SF Markus Elfring, dm-devel, linux-raid, Alasdair Kergon,
Mike Snitzer, Shaohua Li, LKML, kernel-janitors, Julia Lawall
In-Reply-To: <1475180693.1954.1.camel@perches.com>
On Thu, 2016-09-29 at 13:24 -0700, Joe Perches wrote:
> On Thu, 2016-09-29 at 21:43 +0200, Paul Bolle wrote:
> > Why doesn't that regex match on "ORIGIN_HASH_SIZE"?
>
> It does match.
If that regex does match, it being part of a negative test, the
specific checkpatch rule should be silent, shouldn't it?
Paul Bolle
^ permalink raw reply
* Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()
From: Joe Perches @ 2016-09-29 20:24 UTC (permalink / raw)
To: Paul Bolle, Andy Whitcroft
Cc: SF Markus Elfring, dm-devel, linux-raid, Alasdair Kergon,
Mike Snitzer, Shaohua Li, LKML, kernel-janitors, Julia Lawall
In-Reply-To: <1475178181.31297.21.camel@tiscali.nl>
On Thu, 2016-09-29 at 21:43 +0200, Paul Bolle wrote:
> On Thu, 2016-09-29 at 08:01 -0700, Joe Perches wrote:
> > $Constant there is any number and the match regex is
> > any upper case variable.
> Why doesn't that regex match on "ORIGIN_HASH_SIZE"?
It does match.
Did you see my earlier email?
$ ./scripts/checkpatch.pl -f drivers/md/dm-snap.c --show-types --types=alloc_with_multiply
WARNING:ALLOC_WITH_MULTIPLY: Prefer kmalloc_array over kmalloc with multiply
#329: FILE: drivers/md/dm-snap.c:329:
+ _origins = kmalloc(ORIGIN_HASH_SIZE * sizeof(struct list_head),
WARNING:ALLOC_WITH_MULTIPLY: Prefer kmalloc_array over kmalloc with multiply
#338: FILE: drivers/md/dm-snap.c:338:
+ _dm_origins = kmalloc(ORIGIN_HASH_SIZE * sizeof(struct list_head),
total: 0 errors, 2 warnings, 2490 lines checked
^ permalink raw reply
* Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()
From: Paul Bolle @ 2016-09-29 19:43 UTC (permalink / raw)
To: Joe Perches, Andy Whitcroft
Cc: SF Markus Elfring, dm-devel, linux-raid, Alasdair Kergon,
Mike Snitzer, Shaohua Li, LKML, kernel-janitors, Julia Lawall
In-Reply-To: <1475161270.2027.5.camel@perches.com>
On Thu, 2016-09-29 at 08:01 -0700, Joe Perches wrote:
> $Constant there is any number and the match regex is
> any upper case variable.
Why doesn't that regex match on "ORIGIN_HASH_SIZE"?
Paul Bolle
^ permalink raw reply
* Re: [dm-devel] [PATCH 03/10] md/dm-crypt: Rename a jump label in crypt_message()
From: SF Markus Elfring @ 2016-09-29 15:43 UTC (permalink / raw)
To: Theodore Ts'o
Cc: dm-devel, linux-raid, Alasdair Kergon, Mike Snitzer, Shaohua Li,
Julia Lawall, kernel-janitors, LKML
In-Reply-To: <20160929125553.kn45yx6jud35463p@thunk.org>
> In what bizzaro world is the "current Linux coding style convention"
Do you look at the evolution for a document like "CodingStyle"?
>> -
>> -error:
>> +show_warning:
>> DMWARN("unrecognised message received.");
>> return -EINVAL;
>> }
>
> "show_warning" is better than "error"
I got such an impression.
> when the net result of the goto is that the function returns -EINVAL?!?
Do other identifiers fit better for the desired description of "what" and "why"
by jump labels?
> Please give it up with these drive-by shooting of auto-generated patches.
This update step was not auto-generated.
There are further change possibilities where special analysis tools
can help in the corresponding software development.
> You're just embarassing yourself.
Do you find any of my update suggestions worth for further considerations?
Regards,
Markus
^ permalink raw reply
* Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()
From: Joe Perches @ 2016-09-29 15:01 UTC (permalink / raw)
To: Paul Bolle, Andy Whitcroft
Cc: SF Markus Elfring, dm-devel, linux-raid, Alasdair Kergon,
Mike Snitzer, Shaohua Li, LKML, kernel-janitors, Julia Lawall
In-Reply-To: <1475149503.31297.14.camel@tiscali.nl>
On Thu, 2016-09-29 at 13:45 +0200, Paul Bolle wrote:
> On Thu, 2016-09-29 at 13:12 +0200, Paul Bolle wrote:
> > Or did I misread that test?
> I finally did some digging: commit e367455a9f25 ("checkpatch: emit
> fewer kmalloc_array/kcalloc conversion warnings") shows I didn't.
You still misread it a little.
I think it's fine as-is.
$Constant there is any number and the match regex is
any upper case variable.
^ permalink raw reply
* Re: dm snapshot: Use kmalloc_array() in init_origin_hash()
From: Theodore Ts'o @ 2016-09-29 12:59 UTC (permalink / raw)
To: SF Markus Elfring
Cc: Paul Bolle, Andy Whitcroft, Joe Perches, dm-devel, linux-raid,
Alasdair Kergon, Mike Snitzer, Shaohua Li, LKML, kernel-janitors,
Julia Lawall, Paolo Bonzini
In-Reply-To: <80feaa7d-7845-976a-a038-15da80283df8@users.sourceforge.net>
On Thu, Sep 29, 2016 at 01:45:41PM +0200, SF Markus Elfring wrote:
> > We have no hope of fixing Markus' homegrown coccinelle script.
>
> I have got an other impression. I see further possibilities
> to clarify involved communication and software development challenges
> for a few source code search patterns.
>
> How do you think about to discuss the corresponding collateral evolution
> a bit more?
Here's my suggestion:
https://www.youtube.com/watch?v=xAnVNXaa5oA
Regards,
- Ted
^ permalink raw reply
* Re: [dm-devel] [PATCH 05/10] md/dm-crypt: Rename a jump label in crypt_set_key()
From: Theodore Ts'o @ 2016-09-29 12:56 UTC (permalink / raw)
To: SF Markus Elfring
Cc: dm-devel, linux-raid, Alasdair Kergon, Mike Snitzer, Shaohua Li,
Julia Lawall, kernel-janitors, LKML
In-Reply-To: <f89d98a2-fe7e-a9d3-5bdc-2a1943e09049@users.sourceforge.net>
On Wed, Sep 28, 2016 at 05:42:28PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 28 Sep 2016 15:21:18 +0200
>
> Adjust jump labels according to the current Linux coding style convention.
>
> -
> -out:
> +set_memory:
> /* Hex key string not needed after here, so wipe it. */
> memset(key, '0', key_string_len);
Also not "current Linux coding style convetion".
- Ted
^ permalink raw reply
* Re: [dm-devel] [PATCH 03/10] md/dm-crypt: Rename a jump label in crypt_message()
From: Theodore Ts'o @ 2016-09-29 12:55 UTC (permalink / raw)
To: SF Markus Elfring
Cc: dm-devel, linux-raid, Alasdair Kergon, Mike Snitzer, Shaohua Li,
Julia Lawall, kernel-janitors, LKML
In-Reply-To: <db29ff0c-8da9-e7ba-054d-a35f8b4fd2d7@users.sourceforge.net>
On Wed, Sep 28, 2016 at 05:40:14PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 28 Sep 2016 14:54:39 +0200
>
> Adjust a jump label according to the current Linux coding style convention.
In what bizzaro world is the "current Linux coding style convention"
> -
> -error:
> +show_warning:
> DMWARN("unrecognised message received.");
> return -EINVAL;
> }
"show_warning" is better than "error" when the net result of the goto
is that the function returns -EINVAL?!?
Please give it up with these drive-by shooting of auto-generated
patches. You're just embarassing yourself.
- Ted
^ permalink raw reply
* [PATCH] Fix bus error when accessing MBR partition records
From: James Clarke @ 2016-09-29 12:28 UTC (permalink / raw)
To: linux-raid; +Cc: James Clarke
Since the MBR layout only has partition records as 2-byte aligned, the 32-bit
fields in them are not aligned. Thus, they cannot be accessed on some
architectures (such as SPARC) by using a "struct MBR_part_record *" pointer,
as the compiler can assume that the pointer is properly aligned. Instead, the
records must be accessed by going through the MBR struct itself every time.
Signed-off-by: James Clarke <jrtc27@jrtc27.com>
---
super-mbr.c | 6 ++++++
util.c | 14 +++++++-------
2 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/super-mbr.c b/super-mbr.c
index 62b3f03..303dde4 100644
--- a/super-mbr.c
+++ b/super-mbr.c
@@ -57,6 +57,9 @@ static void examine_mbr(struct supertype *st, char *homehost)
printf(" MBR Magic : %04x\n", sb->magic);
for (i = 0; i < MBR_PARTITIONS; i++)
+ /* Have to make every access through sb rather than using a pointer to
+ * the partition table (or an entry), since the entries are not
+ * properly aligned. */
if (sb->parts[i].blocks_num)
printf("Partition[%d] : %12lu sectors at %12lu (type %02x)\n",
i,
@@ -151,6 +154,9 @@ static void getinfo_mbr(struct supertype *st, struct mdinfo *info, char *map)
info->component_size = 0;
for (i = 0; i < MBR_PARTITIONS ; i++)
+ /* Have to make every access through sb rather than using a pointer to
+ * the partition table (or an entry), since the entries are not
+ * properly aligned. */
if (sb->parts[i].blocks_num) {
unsigned long last =
(unsigned long)__le32_to_cpu(sb->parts[i].blocks_num)
diff --git a/util.c b/util.c
index a238a21..08adbd5 100644
--- a/util.c
+++ b/util.c
@@ -1412,7 +1412,6 @@ static int get_gpt_last_partition_end(int fd, unsigned long long *endofpart)
static int get_last_partition_end(int fd, unsigned long long *endofpart)
{
struct MBR boot_sect;
- struct MBR_part_record *part;
unsigned long long curr_part_end;
unsigned part_nr;
int retval = 0;
@@ -1429,21 +1428,22 @@ static int get_last_partition_end(int fd, unsigned long long *endofpart)
if (boot_sect.magic == MBR_SIGNATURE_MAGIC) {
retval = 1;
/* found the correct signature */
- part = boot_sect.parts;
for (part_nr = 0; part_nr < MBR_PARTITIONS; part_nr++) {
+ /* Have to make every access through boot_sect rather than using a
+ * pointer to the partition table (or an entry), since the entries
+ * are not properly aligned. */
+
/* check for GPT type */
- if (part->part_type == MBR_GPT_PARTITION_TYPE) {
+ if (boot_sect.parts[part_nr].part_type == MBR_GPT_PARTITION_TYPE) {
retval = get_gpt_last_partition_end(fd, endofpart);
break;
}
/* check the last used lba for the current partition */
- curr_part_end = __le32_to_cpu(part->first_sect_lba) +
- __le32_to_cpu(part->blocks_num);
+ curr_part_end = __le32_to_cpu(boot_sect.parts[part_nr].first_sect_lba) +
+ __le32_to_cpu(boot_sect.parts[part_nr].blocks_num);
if (curr_part_end > *endofpart)
*endofpart = curr_part_end;
-
- part++;
}
} else {
/* Unknown partition table */
--
2.10.0
^ permalink raw reply related
* Re: dm snapshot: Use kmalloc_array() in init_origin_hash()
From: SF Markus Elfring @ 2016-09-29 11:45 UTC (permalink / raw)
To: Paul Bolle
Cc: Andy Whitcroft, Joe Perches, dm-devel, linux-raid,
Alasdair Kergon, Mike Snitzer, Shaohua Li, LKML, kernel-janitors,
Julia Lawall, Paolo Bonzini
In-Reply-To: <1475142864.31297.5.camel@tiscali.nl>
> We have no hope of fixing Markus' homegrown coccinelle script.
I have got an other impression. I see further possibilities
to clarify involved communication and software development challenges
for a few source code search patterns.
How do you think about to discuss the corresponding collateral evolution
a bit more?
Are there any more constraints to consider?
Regards,
Markus
^ permalink raw reply
* Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()
From: Paul Bolle @ 2016-09-29 11:45 UTC (permalink / raw)
To: Joe Perches, Andy Whitcroft
Cc: SF Markus Elfring, dm-devel, linux-raid, Alasdair Kergon,
Mike Snitzer, Shaohua Li, LKML, kernel-janitors, Julia Lawall
In-Reply-To: <1475147577.31297.11.camel@tiscali.nl>
On Thu, 2016-09-29 at 13:12 +0200, Paul Bolle wrote:
> Or did I misread that test?
I finally did some digging: commit e367455a9f25 ("checkpatch: emit
fewer kmalloc_array/kcalloc conversion warnings") shows I didn't.
Paul Bolle
^ permalink raw reply
* Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()
From: Paul Bolle @ 2016-09-29 11:12 UTC (permalink / raw)
To: Joe Perches, Andy Whitcroft
Cc: SF Markus Elfring, dm-devel, linux-raid, Alasdair Kergon,
Mike Snitzer, Shaohua Li, LKML, kernel-janitors, Julia Lawall
In-Reply-To: <1475143376.1946.2.camel@perches.com>
On Thu, 2016-09-29 at 03:02 -0700, Joe Perches wrote:
> What's the false positive?
>
> I get:
>
> $ ./scripts/checkpatch.pl -f drivers/md/dm-snap.c --show-types --types=alloc_with_multiply
> WARNING:ALLOC_WITH_MULTIPLY: Prefer kmalloc_array over kmalloc with multiply
> #329: FILE: drivers/md/dm-snap.c:329:
> + _origins = kmalloc(ORIGIN_HASH_SIZE * sizeof(struct list_head),
>
> WARNING:ALLOC_WITH_MULTIPLY: Prefer kmalloc_array over kmalloc with multiply
> #338: FILE: drivers/md/dm-snap.c:338:
> + _dm_origins = kmalloc(ORIGIN_HASH_SIZE * sizeof(struct list_head),
>
> total: 0 errors, 2 warnings, 2490 lines checked
>
> NOTE: For some of the reported defects, checkpatch may be able to
> mechanically convert to the typical style using --fix or --fix-inplace.
>
> drivers/md/dm-snap.c has style problems, please review.
>
> NOTE: Used message types: ALLOC_WITH_MULTIPLY
>
> NOTE: If any of the errors are false positives, please report
> them to the maintainer, see CHECKPATCH in MAINTAINERS.
It seems it was intended to be silent about multiplying with constants,
where things that look like preprocessor defines are also considered
constants. Or did I misread that test?
Paul Bolle
^ permalink raw reply
* Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()
From: Joe Perches @ 2016-09-29 10:02 UTC (permalink / raw)
To: Paul Bolle, Andy Whitcroft
Cc: SF Markus Elfring, dm-devel, linux-raid, Alasdair Kergon,
Mike Snitzer, Shaohua Li, LKML, kernel-janitors, Julia Lawall
In-Reply-To: <1475142864.31297.5.camel@tiscali.nl>
On Thu, 2016-09-29 at 11:54 +0200, Paul Bolle wrote:
> Andy, Joe,
>
> On Thu, 2016-09-29 at 11:07 +0200, SF Markus Elfring wrote:
> > * Multiplications for the size determination of memory allocations
> > indicated that array data structures should be processed.
> > Thus use the corresponding function "kmalloc_array".
> >
> > This issue was detected by using the Coccinelle software.
>
>
> We have no hope of fixing Markus' homegrown coccinelle script. But we
> could try to fix the checkpatch false positive here.
What's the false positive?
I get:
$ ./scripts/checkpatch.pl -f drivers/md/dm-snap.c --show-types --types=alloc_with_multiply
WARNING:ALLOC_WITH_MULTIPLY: Prefer kmalloc_array over kmalloc with multiply
#329: FILE: drivers/md/dm-snap.c:329:
+ _origins = kmalloc(ORIGIN_HASH_SIZE * sizeof(struct list_head),
WARNING:ALLOC_WITH_MULTIPLY: Prefer kmalloc_array over kmalloc with multiply
#338: FILE: drivers/md/dm-snap.c:338:
+ _dm_origins = kmalloc(ORIGIN_HASH_SIZE * sizeof(struct list_head),
total: 0 errors, 2 warnings, 2490 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
drivers/md/dm-snap.c has style problems, please review.
NOTE: Used message types: ALLOC_WITH_MULTIPLY
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
^ permalink raw reply
* Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()
From: Paul Bolle @ 2016-09-29 9:54 UTC (permalink / raw)
To: Andy Whitcroft, Joe Perches
Cc: SF Markus Elfring, dm-devel, linux-raid, Alasdair Kergon,
Mike Snitzer, Shaohua Li, LKML, kernel-janitors, Julia Lawall
In-Reply-To: <3861d349-48fd-162b-a749-83e007f70b41@users.sourceforge.net>
Andy, Joe,
On Thu, 2016-09-29 at 11:07 +0200, SF Markus Elfring wrote:
> * Multiplications for the size determination of memory allocations
> indicated that array data structures should be processed.
> Thus use the corresponding function "kmalloc_array".
>
> This issue was detected by using the Coccinelle software.
We have no hope of fixing Markus' homegrown coccinelle script. But we
could try to fix the checkpatch false positive here. Something like:
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 206a6b3..b47201d 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5693,7 +5693,7 @@ sub process {
$r2 = $a1;
}
if ($r1 !~ /^sizeof\b/ && $r2 =~ /^sizeof\s*\S/ &&
- !($r1 =~ /^$Constant$/ || $r1 =~ /^[A-Z_][A-Z0-9_]*$/)) {
+ !($r1 =~ /^$Constant$/ || $r1 =~ /^[A-Z_][A-Z0-9_]*\b/)) {
if (WARN("ALLOC_WITH_MULTIPLY",
"Prefer $newfunc over $oldfunc with multiply\n" . $herecurr) &&
$fix) {
Does that work for you too?
> --- a/drivers/md/dm-snap.c
> +++ b/drivers/md/dm-snap.c
> @@ -326,8 +326,9 @@ static int init_origin_hash(void)
> {
> int i;
>
> - _origins = kmalloc(ORIGIN_HASH_SIZE * sizeof(struct list_head),
> - GFP_KERNEL);
> + _origins = kmalloc_array(ORIGIN_HASH_SIZE,
> + sizeof(*_origins),
> + GFP_KERNEL);
> if (!_origins) {
> DMERR("unable to allocate memory for _origins");
> return -ENOMEM;
> @@ -335,8 +336,9 @@ static int init_origin_hash(void)
> for (i = 0; i < ORIGIN_HASH_SIZE; i++)
> INIT_LIST_HEAD(_origins + i);
>
> - _dm_origins = kmalloc(ORIGIN_HASH_SIZE * sizeof(struct list_head),
> - GFP_KERNEL);
> + _dm_origins = kmalloc_array(ORIGIN_HASH_SIZE,
> + sizeof(*_dm_origins),
> + GFP_KERNEL);
> if (!_dm_origins) {
> DMERR("unable to allocate memory for _dm_origins");
> kfree(_origins);
Thanks,
Paul Bolle
^ permalink raw reply related
* [PATCH 10/10] dm snapshot: Delete five unwanted spaces behind "list_for_each_entry"
From: SF Markus Elfring @ 2016-09-29 9:18 UTC (permalink / raw)
To: dm-devel, linux-raid, Alasdair Kergon, Mike Snitzer, Shaohua Li
Cc: LKML, kernel-janitors, Julia Lawall
In-Reply-To: <080668d9-1e1e-e208-f9ea-ff718e8070e5@users.sourceforge.net>
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 29 Sep 2016 10:14:32 +0200
The script "checkpatch.pl" can point information out like the following.
WARNING: space prohibited between function name and open parenthesis '('
Thus fix the affected source code places.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/md/dm-snap.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/md/dm-snap.c b/drivers/md/dm-snap.c
index 7593986..1310652 100644
--- a/drivers/md/dm-snap.c
+++ b/drivers/md/dm-snap.c
@@ -366,7 +366,7 @@ static struct origin *__lookup_origin(struct block_device *origin)
struct origin *o;
ol = &_origins[origin_hash(origin)];
- list_for_each_entry (o, ol, hash_list)
+ list_for_each_entry(o, ol, hash_list)
if (bdev_equal(o->bdev, origin))
return o;
@@ -385,7 +385,7 @@ static struct dm_origin *__lookup_dm_origin(struct block_device *origin)
struct dm_origin *o;
ol = &_dm_origins[origin_hash(origin)];
- list_for_each_entry (o, ol, hash_list)
+ list_for_each_entry(o, ol, hash_list)
if (bdev_equal(o->dev->bdev, origin))
return o;
@@ -625,7 +625,7 @@ static void dm_exception_table_exit(struct dm_exception_table *et,
for (i = 0; i < size; i++) {
slot = et->table + i;
- list_for_each_entry_safe (ex, next, slot, hash_list)
+ list_for_each_entry_safe(ex, next, slot, hash_list)
kmem_cache_free(mem, ex);
}
@@ -653,7 +653,7 @@ static struct dm_exception *dm_lookup_exception(struct dm_exception_table *et,
struct dm_exception *e;
slot = &et->table[exception_hash(et, chunk)];
- list_for_each_entry (e, slot, hash_list)
+ list_for_each_entry(e, slot, hash_list)
if (chunk >= e->old_chunk &&
chunk <= e->old_chunk + dm_consecutive_chunk_count(e))
return e;
@@ -2068,7 +2068,7 @@ static int __origin_write(struct list_head *snapshots, sector_t sector,
chunk_t chunk;
/* Do all the snapshots on this origin */
- list_for_each_entry (snap, snapshots, list) {
+ list_for_each_entry(snap, snapshots, list) {
/*
* Don't make new exceptions in a merging snapshot
* because it has effectively been deleted
--
2.10.0
^ 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