All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dan Carpenter <dan.carpenter@oracle.com>
To: Gao Xiang <gaoxiang25@huawei.com>
Cc: Chao Yu <yuchao0@huawei.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	devel@driverdev.osuosl.org, linux-erofs@lists.ozlabs.org,
	Chao Yu <chao@kernel.org>, LKML <linux-kernel@vger.kernel.org>,
	weidu.du@huawei.com, Fang Wei <fangwei1@huawei.com>,
	Miao Xie <miaoxie@huawei.com>
Subject: Re: [PATCH 3/5] staging: erofs: decrease the shrink count in erofs_workgroup_get
Date: Wed, 16 Jan 2019 13:45:53 +0300	[thread overview]
Message-ID: <20190116104553.GE4482@kadam> (raw)
In-Reply-To: <20190116085956.21004-3-gaoxiang25@huawei.com>

On Wed, Jan 16, 2019 at 04:59:54PM +0800, Gao Xiang wrote:
> It is more suitable to update in erofs_workgroup_get since
> it's actually the one matched with erofs_workgroup_put.
> 

This patch is fine.  No need to resend.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

But for future reference, I found the commit message a bit confusing.
Ideally, I would understand basically what the commit does and why
without needing to read the diff.

I mostly just read the subject or the commit message, either or, instead
of reading both.  (This is not really true.)  My email client looks like
this.  (Also not true).

https://marc.info/?l=linux-driver-devel&m=154762932712086&w=2

While none of that was strictly true, it is a little bit true-ish...
So please, assume that some people will start reading the commit message
without reading the subject.

I sort of thought from reading the commit message that it was a bugfix
or a behavior change.  A better commit message would be:

staging: erofs: move shrink accounting inside the function

This patch moves the &erofs_global_shrink_cnt accounting from the caller
to erofs_workgroup_get().  It's cleaner and it matches erofs_workgroup_put()
better.  No behavior change.

regards,
dan carpenter


WARNING: multiple messages have this Message-ID (diff)
From: dan.carpenter@oracle.com (Dan Carpenter)
Subject: [PATCH 3/5] staging: erofs: decrease the shrink count in erofs_workgroup_get
Date: Wed, 16 Jan 2019 13:45:53 +0300	[thread overview]
Message-ID: <20190116104553.GE4482@kadam> (raw)
In-Reply-To: <20190116085956.21004-3-gaoxiang25@huawei.com>

On Wed, Jan 16, 2019@04:59:54PM +0800, Gao Xiang wrote:
> It is more suitable to update in erofs_workgroup_get since
> it's actually the one matched with erofs_workgroup_put.
> 

This patch is fine.  No need to resend.

Reviewed-by: Dan Carpenter <dan.carpenter at oracle.com>

But for future reference, I found the commit message a bit confusing.
Ideally, I would understand basically what the commit does and why
without needing to read the diff.

I mostly just read the subject or the commit message, either or, instead
of reading both.  (This is not really true.)  My email client looks like
this.  (Also not true).

https://marc.info/?l=linux-driver-devel&m=154762932712086&w=2

While none of that was strictly true, it is a little bit true-ish...
So please, assume that some people will start reading the commit message
without reading the subject.

I sort of thought from reading the commit message that it was a bugfix
or a behavior change.  A better commit message would be:

staging: erofs: move shrink accounting inside the function

This patch moves the &erofs_global_shrink_cnt accounting from the caller
to erofs_workgroup_get().  It's cleaner and it matches erofs_workgroup_put()
better.  No behavior change.

regards,
dan carpenter

  parent reply	other threads:[~2019-01-16 10:46 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-16  8:59 [PATCH 1/5] staging: erofs: sunset erofs_workstation_cleanup_all Gao Xiang
2019-01-16  8:59 ` Gao Xiang
2019-01-16  8:59 ` [PATCH 2/5] staging: erofs: localize erofs_workgroup_get Gao Xiang
2019-01-16  8:59   ` Gao Xiang
2019-01-16  9:20   ` Chao Yu
2019-01-16  9:20     ` Chao Yu
2019-01-16  8:59 ` [PATCH 3/5] staging: erofs: decrease the shrink count in erofs_workgroup_get Gao Xiang
2019-01-16  8:59   ` Gao Xiang
2019-01-16  9:27   ` Chao Yu
2019-01-16  9:27     ` Chao Yu
2019-01-16 10:45   ` Dan Carpenter [this message]
2019-01-16 10:45     ` Dan Carpenter
2019-01-16 13:01     ` Gao Xiang
2019-01-16 13:01       ` Gao Xiang
2019-01-16 13:10       ` [PATCH v2 3/5] staging: erofs: move shrink accounting inside the function Gao Xiang
2019-01-16 13:10         ` Gao Xiang
2019-01-16  8:59 ` [PATCH 4/5] staging: erofs: staticize erofs_shrink_count, erofs_shrink_scan Gao Xiang
2019-01-16  8:59   ` Gao Xiang
2019-01-16  9:28   ` Chao Yu
2019-01-16  9:28     ` Chao Yu
2019-01-16  8:59 ` [PATCH 5/5] staging: erofs: drop the extern prefix for function definitions Gao Xiang
2019-01-16  8:59   ` Gao Xiang
2019-01-16  9:29   ` Chao Yu
2019-01-16  9:29     ` Chao Yu
2019-01-16  9:19 ` [PATCH 1/5] staging: erofs: sunset erofs_workstation_cleanup_all Chao Yu
2019-01-16  9:19   ` Chao Yu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190116104553.GE4482@kadam \
    --to=dan.carpenter@oracle.com \
    --cc=chao@kernel.org \
    --cc=devel@driverdev.osuosl.org \
    --cc=fangwei1@huawei.com \
    --cc=gaoxiang25@huawei.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-erofs@lists.ozlabs.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=miaoxie@huawei.com \
    --cc=weidu.du@huawei.com \
    --cc=yuchao0@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.