* One patch just review the code [not found] <705044700.17914512.1407222497904.JavaMail.zimbra@redhat.com> @ 2014-08-05 7:12 ` Xiao Ni 2014-08-06 6:44 ` NeilBrown 0 siblings, 1 reply; 4+ messages in thread From: Xiao Ni @ 2014-08-05 7:12 UTC (permalink / raw) To: linux-raid; +Cc: Jes Sorensen Hi all I'm reading the code of md. I find there is a problem. I know now there are arrays mark[SYNC_MARKS] mark_cnt[SYNC_MARKS] store the information about how many sectors finish recovery and the moment. 7825 currspeed = ((unsigned long)(io_sectors-mddev->resync_mark_cnt))/2 7826 /((jiffies-mddev->resync_mark)/HZ +1) +1; But when calculate the speed of recovery, the sectors used to calculate contains the sectors which are not finished recovery. When assign value to mark_cnt[next], it subtract the sectors which don't finish recovery. So I think when calculate the recovery speed we should subtract the sectors not finishing recovery too. 7638 mark_cnt[next] = io_sectors - atomic_read(&mddev->recovery_active); So I try to modify and the patch is: --- linux-stable/drivers/md/md.c 2014-07-30 14:36:37.327535805 +0800 +++ fix/md.c 2014-07-31 16:40:57.151493177 +0800 @@ -7652,7 +7652,7 @@ */ cond_resched(); - currspeed = ((unsigned long)(io_sectors-mddev->resync_mark_cnt))/2 + currspeed = ((unsigned long)(io_sectors-atomic_read(&mddev->recovery_active)-mddev->resync_mark_cnt))/2 /((jiffies-mddev->resync_mark)/HZ +1) +1; if (currspeed > speed_min(mddev)) { Am I right? Best Regards Xiao ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: One patch just review the code 2014-08-05 7:12 ` One patch just review the code Xiao Ni @ 2014-08-06 6:44 ` NeilBrown 2014-08-07 10:06 ` Xiao Ni 0 siblings, 1 reply; 4+ messages in thread From: NeilBrown @ 2014-08-06 6:44 UTC (permalink / raw) To: Xiao Ni; +Cc: linux-raid, Jes Sorensen [-- Attachment #1: Type: text/plain, Size: 1585 bytes --] On Tue, 5 Aug 2014 03:12:07 -0400 (EDT) Xiao Ni <xni@redhat.com> wrote: > Hi all > > I'm reading the code of md. I find there is a problem. I know now there are arrays mark[SYNC_MARKS] mark_cnt[SYNC_MARKS] > store the information about how many sectors finish recovery and the moment. > > > 7825 currspeed = ((unsigned long)(io_sectors-mddev->resync_mark_cnt))/2 > 7826 /((jiffies-mddev->resync_mark)/HZ +1) +1; > > But when calculate the speed of recovery, the sectors used to calculate contains > the sectors which are not finished recovery. > > When assign value to mark_cnt[next], it subtract the sectors which don't finish recovery. > So I think when calculate the recovery speed we should subtract the sectors not finishing > recovery too. > > 7638 mark_cnt[next] = io_sectors - atomic_read(&mddev->recovery_active); > > So I try to modify and the patch is: > > --- linux-stable/drivers/md/md.c 2014-07-30 14:36:37.327535805 +0800 > +++ fix/md.c 2014-07-31 16:40:57.151493177 +0800 > @@ -7652,7 +7652,7 @@ > */ > cond_resched(); > > - currspeed = ((unsigned long)(io_sectors-mddev->resync_mark_cnt))/2 > + currspeed = ((unsigned long)(io_sectors-atomic_read(&mddev->recovery_active)-mddev->resync_mark_cnt))/2 > /((jiffies-mddev->resync_mark)/HZ +1) +1; > > if (currspeed > speed_min(mddev)) { > > Am I right? Yes, that looks right. If you create a properly formatted patch, and wrap that long line nicely I'll apply it. Thanks, NeilBrown [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 828 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: One patch just review the code 2014-08-06 6:44 ` NeilBrown @ 2014-08-07 10:06 ` Xiao Ni 2014-08-07 10:26 ` NeilBrown 0 siblings, 1 reply; 4+ messages in thread From: Xiao Ni @ 2014-08-07 10:06 UTC (permalink / raw) To: NeilBrown; +Cc: linux-raid, Jes Sorensen ----- Original Message ----- > From: "NeilBrown" <neilb@suse.de> > To: "Xiao Ni" <xni@redhat.com> > Cc: linux-raid@vger.kernel.org, "Jes Sorensen" <jes.sorensen@redhat.com> > Sent: Wednesday, August 6, 2014 2:44:20 PM > Subject: Re: One patch just review the code > > On Tue, 5 Aug 2014 03:12:07 -0400 (EDT) Xiao Ni <xni@redhat.com> wrote: > > > Hi all > > > > I'm reading the code of md. I find there is a problem. I know now there > > are arrays mark[SYNC_MARKS] mark_cnt[SYNC_MARKS] > > store the information about how many sectors finish recovery and the > > moment. > > > > > > 7825 currspeed = ((unsigned > > long)(io_sectors-mddev->resync_mark_cnt))/2 > > 7826 /((jiffies-mddev->resync_mark)/HZ +1) +1; > > > > But when calculate the speed of recovery, the sectors used to calculate > > contains > > the sectors which are not finished recovery. > > > > When assign value to mark_cnt[next], it subtract the sectors which don't > > finish recovery. > > So I think when calculate the recovery speed we should subtract the sectors > > not finishing > > recovery too. > > > > 7638 mark_cnt[next] = io_sectors - > > atomic_read(&mddev->recovery_active); > > > > So I try to modify and the patch is: > > > > --- linux-stable/drivers/md/md.c 2014-07-30 14:36:37.327535805 +0800 > > +++ fix/md.c 2014-07-31 16:40:57.151493177 +0800 > > @@ -7652,7 +7652,7 @@ > > */ > > cond_resched(); > > > > - currspeed = ((unsigned long)(io_sectors-mddev->resync_mark_cnt))/2 > > + currspeed = ((unsigned > > long)(io_sectors-atomic_read(&mddev->recovery_active)-mddev->resync_mark_cnt))/2 > > /((jiffies-mddev->resync_mark)/HZ +1) +1; > > > > if (currspeed > speed_min(mddev)) { > > > > Am I right? > > Yes, that looks right. > If you create a properly formatted patch, and wrap that long line nicely I'll > apply it. > > Thanks, > NeilBrown > I definite a new variable, do you allow me to do by this way? Signed-off-by: Xiao Ni <xni@redhat.com> diff -urN linux-stable/drivers/md/md.c fix/md.c --- linux-stable/drivers/md/md.c 2014-07-30 14:36:37.327535805 +0800 +++ fix/md.c 2014-08-07 16:07:12.559503942 +0800 @@ -7376,7 +7376,7 @@ struct mddev *mddev2; unsigned int currspeed = 0, window; - sector_t max_sectors,j, io_sectors; + sector_t max_sectors,j, io_sectors, recovery_done; unsigned long mark[SYNC_MARKS]; unsigned long update_time; sector_t mark_cnt[SYNC_MARKS]; @@ -7652,7 +7652,8 @@ */ cond_resched(); - currspeed = ((unsigned long)(io_sectors-mddev->resync_mark_cnt))/2 + recovery_done = io_sectors - atomic_read(&mddev->recovery_active); + currspeed = ((unsigned long)recovery_done - mddev->resync_mark_cnt)/2 /((jiffies-mddev->resync_mark)/HZ +1) +1; if (currspeed > speed_min(mddev)) { ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: One patch just review the code 2014-08-07 10:06 ` Xiao Ni @ 2014-08-07 10:26 ` NeilBrown 0 siblings, 0 replies; 4+ messages in thread From: NeilBrown @ 2014-08-07 10:26 UTC (permalink / raw) To: Xiao Ni; +Cc: linux-raid, Jes Sorensen [-- Attachment #1: Type: text/plain, Size: 3546 bytes --] On Thu, 7 Aug 2014 06:06:19 -0400 (EDT) Xiao Ni <xni@redhat.com> wrote: > > > ----- Original Message ----- > > From: "NeilBrown" <neilb@suse.de> > > To: "Xiao Ni" <xni@redhat.com> > > Cc: linux-raid@vger.kernel.org, "Jes Sorensen" <jes.sorensen@redhat.com> > > Sent: Wednesday, August 6, 2014 2:44:20 PM > > Subject: Re: One patch just review the code > > > > On Tue, 5 Aug 2014 03:12:07 -0400 (EDT) Xiao Ni <xni@redhat.com> wrote: > > > > > Hi all > > > > > > I'm reading the code of md. I find there is a problem. I know now there > > > are arrays mark[SYNC_MARKS] mark_cnt[SYNC_MARKS] > > > store the information about how many sectors finish recovery and the > > > moment. > > > > > > > > > 7825 currspeed = ((unsigned > > > long)(io_sectors-mddev->resync_mark_cnt))/2 > > > 7826 /((jiffies-mddev->resync_mark)/HZ +1) +1; > > > > > > But when calculate the speed of recovery, the sectors used to calculate > > > contains > > > the sectors which are not finished recovery. > > > > > > When assign value to mark_cnt[next], it subtract the sectors which don't > > > finish recovery. > > > So I think when calculate the recovery speed we should subtract the sectors > > > not finishing > > > recovery too. > > > > > > 7638 mark_cnt[next] = io_sectors - > > > atomic_read(&mddev->recovery_active); > > > > > > So I try to modify and the patch is: > > > > > > --- linux-stable/drivers/md/md.c 2014-07-30 14:36:37.327535805 +0800 > > > +++ fix/md.c 2014-07-31 16:40:57.151493177 +0800 > > > @@ -7652,7 +7652,7 @@ > > > */ > > > cond_resched(); > > > > > > - currspeed = ((unsigned long)(io_sectors-mddev->resync_mark_cnt))/2 > > > + currspeed = ((unsigned > > > long)(io_sectors-atomic_read(&mddev->recovery_active)-mddev->resync_mark_cnt))/2 > > > /((jiffies-mddev->resync_mark)/HZ +1) +1; > > > > > > if (currspeed > speed_min(mddev)) { > > > > > > Am I right? > > > > Yes, that looks right. > > If you create a properly formatted patch, and wrap that long line nicely I'll > > apply it. > > > > Thanks, > > NeilBrown > > > > I definite a new variable, do you allow me to do by this way? Certainly, nothing wrong with a new variable. But when you post a patch, please create a new email message with a short description of the patch as the subject, any extra details or explanation in the body, then the signed-off-by line and the patch. Then I can just apply that email without editing it. Thanks, NeilBrown > > Signed-off-by: Xiao Ni <xni@redhat.com> > > diff -urN linux-stable/drivers/md/md.c fix/md.c > --- linux-stable/drivers/md/md.c 2014-07-30 14:36:37.327535805 +0800 > +++ fix/md.c 2014-08-07 16:07:12.559503942 +0800 > @@ -7376,7 +7376,7 @@ > struct mddev *mddev2; > unsigned int currspeed = 0, > window; > - sector_t max_sectors,j, io_sectors; > + sector_t max_sectors,j, io_sectors, recovery_done; > unsigned long mark[SYNC_MARKS]; > unsigned long update_time; > sector_t mark_cnt[SYNC_MARKS]; > @@ -7652,7 +7652,8 @@ > */ > cond_resched(); > > - currspeed = ((unsigned long)(io_sectors-mddev->resync_mark_cnt))/2 > + recovery_done = io_sectors - atomic_read(&mddev->recovery_active); > + currspeed = ((unsigned long)recovery_done - mddev->resync_mark_cnt)/2 > /((jiffies-mddev->resync_mark)/HZ +1) +1; > > if (currspeed > speed_min(mddev)) { [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 828 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-08-07 10:26 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- [not found] <705044700.17914512.1407222497904.JavaMail.zimbra@redhat.com> 2014-08-05 7:12 ` One patch just review the code Xiao Ni 2014-08-06 6:44 ` NeilBrown 2014-08-07 10:06 ` Xiao Ni 2014-08-07 10:26 ` NeilBrown
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).