For some time now I''m watching onnv-notify at opensolaris.org. I''m mostly interested in ZFS changes, so I ''hg export <revision>'' on my downloaded repository to take a look. The problem is that it is often really hard to tell what the change really does. Let me give an example: Repository: /hg/onnv/onnv-gate Latest revision: 24fbf2d7a5d7d3a91e33eeff0360a238569231bd Total changesets: 1 Log message: PSARC 2007/197 ZFS hotplug PSARC 2007/283 FMA for ZFS Phase 2 6401126 ZFS DE should verify that diagnosis is still valid before solving cases 6500545 ZFS does not handle changes in devids 6508521 zpool online should warn when it is being used incorrectly 6509807 ZFS checksum ereports are not being posted 6514712 zfs_nicenum() doesn''t work with perfectly-sized buffers 6520510 media state doesn''t get updated properly on device removal 6520513 ZFS should have better support for device removal 6520514 vdev state should be controlled through a single ioctl() 6520519 ZFS should diagnose faulty devices 6520947 ZFS DE should close cases which no longer apply 6521393 ZFS case timeout should be FMD_TYPE_TIME 6521624 fmd_hash_walk() can dump core when given a bad address 6521946 ZFS DE needlessly subscribes to faults 6522085 ZFS dictionary files contain spelling errors 6523185 vdev_reopen() doesn''t correctly propagate state 6523555 ''zpool online'' should be less chatty unless something goes wrong 6527379 zpool(1M) should not try to open faulted devices 6527700 ZFS should post a sysevent when topology changes 6528194 lofi should support force unmap and DKIO_DEV_GONE 6528732 ZFS should store physical device path in addition to /dev path 6532635 ZFS keeps devices open unnecessarily 6532979 bad argument to ZFS_IOC_VDEV_ATTACH can panic system 6567983 deadlock with spa_scrub_thread() and spa_namespace_lock There is huge number of changes here. Now I''m trying to take only "6532635 ZFS keeps devices open unnecessarily" part for inclusion in FreeBSD 7.0-RELEASE. I looked at the patch, I read explanation for 6532635, I know part of this change is vdev_set_state() modification, but is it the only function modified within this single bug? Another example. I spent a lot of time trying to track down a lock leak. This was a lock leak in zap_micro.c. After I found it, I realized that it is already fixed in OpenSolaris: http://src.opensolaris.org/source/diff/onnv/onnv-gate/usr/src/uts/common/fs/zfs/zap_micro.c?r2=4831&r1=2856 by this change: Repository: /hg/onnv/onnv-gate Latest revision: 41ec732c6d9fc141be2c6cdccf2f6980f93bd061 Total changesets: 1 Log message: 6584470 zdb needs to initialize the bpl_lock mutex 6583739 libzpool should check for properly initialized mutexes 6548010 unbalanced mutex_init/mutex_destroy issues in zfs 6502263 ZFS needs some more FreeBSD porting love Contributed by Pawel Dawidek 6576827 multiple calls to spa_activate() can end up reinitializing all its mutexes 6576830 certain spa mutexes and condition variables need some love I don''t see anything in the log message which can be used as a hint for this fix. I''d love to be able to see patches for each bug separately, but I don''t think it is possible currently. Do you guys commit the changes in smaller chunks at Sun and export them to mercurial in bigger chunks, or this is bascially how you work now? In FreeBSD we always try to keep unrelated changes in separate commits and this is really useful. I think my question is if I can read the changes separately somehow? If the answer is no, maybe it is worth thinking about improving it a bit in the future? -- Pawel Jakub Dawidek http://www.wheel.pl pjd at FreeBSD.org http://www.FreeBSD.org FreeBSD committer Am I Evil? Yes, I Am! -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 187 bytes Desc: not available URL: <http://mail.opensolaris.org/pipermail/zfs-code/attachments/20071101/d118dd2f/attachment.bin>
Pawel has a good point. Back in the days when UFS ruled we used to try to have one bug fix per putback. The main reason was ease of backporting but it also made easier understanding the webrev and the fix later; enabling backouts, etc Not that we always did it, but that was the goal. The cost to this is the overhead of many more putbacks, workspaces etc. Having said that I currently have a wad of a least 4 ZIL changes in the pipe. Neil. Pawel Jakub Dawidek wrote:> For some time now I''m watching onnv-notify at opensolaris.org. I''m mostly > interested in ZFS changes, so I ''hg export <revision>'' on my downloaded > repository to take a look. The problem is that it is often really hard > to tell what the change really does. Let me give an example: > > Repository: /hg/onnv/onnv-gate > Latest revision: 24fbf2d7a5d7d3a91e33eeff0360a238569231bd > Total changesets: 1 > Log message: > PSARC 2007/197 ZFS hotplug > PSARC 2007/283 FMA for ZFS Phase 2 > 6401126 ZFS DE should verify that diagnosis is still valid before solving cases > 6500545 ZFS does not handle changes in devids > 6508521 zpool online should warn when it is being used incorrectly > 6509807 ZFS checksum ereports are not being posted > 6514712 zfs_nicenum() doesn''t work with perfectly-sized buffers > 6520510 media state doesn''t get updated properly on device removal > 6520513 ZFS should have better support for device removal > 6520514 vdev state should be controlled through a single ioctl() > 6520519 ZFS should diagnose faulty devices > 6520947 ZFS DE should close cases which no longer apply > 6521393 ZFS case timeout should be FMD_TYPE_TIME > 6521624 fmd_hash_walk() can dump core when given a bad address > 6521946 ZFS DE needlessly subscribes to faults > 6522085 ZFS dictionary files contain spelling errors > 6523185 vdev_reopen() doesn''t correctly propagate state > 6523555 ''zpool online'' should be less chatty unless something goes wrong > 6527379 zpool(1M) should not try to open faulted devices > 6527700 ZFS should post a sysevent when topology changes > 6528194 lofi should support force unmap and DKIO_DEV_GONE > 6528732 ZFS should store physical device path in addition to /dev path > 6532635 ZFS keeps devices open unnecessarily > 6532979 bad argument to ZFS_IOC_VDEV_ATTACH can panic system > 6567983 deadlock with spa_scrub_thread() and spa_namespace_lock > > There is huge number of changes here. Now I''m trying to take only > "6532635 ZFS keeps devices open unnecessarily" part for inclusion in > FreeBSD 7.0-RELEASE. I looked at the patch, I read explanation for > 6532635, I know part of this change is vdev_set_state() modification, > but is it the only function modified within this single bug? > > Another example. I spent a lot of time trying to track down a lock leak. > This was a lock leak in zap_micro.c. After I found it, I realized that > it is already fixed in OpenSolaris: > > http://src.opensolaris.org/source/diff/onnv/onnv-gate/usr/src/uts/common/fs/zfs/zap_micro.c?r2=4831&r1=2856 > > by this change: > > Repository: /hg/onnv/onnv-gate > Latest revision: 41ec732c6d9fc141be2c6cdccf2f6980f93bd061 > Total changesets: 1 > Log message: > 6584470 zdb needs to initialize the bpl_lock mutex > 6583739 libzpool should check for properly initialized mutexes > 6548010 unbalanced mutex_init/mutex_destroy issues in zfs > 6502263 ZFS needs some more FreeBSD porting love > Contributed by Pawel Dawidek > 6576827 multiple calls to spa_activate() can end up reinitializing all its mutexes > 6576830 certain spa mutexes and condition variables need some love > > I don''t see anything in the log message which can be used as a hint for > this fix. > > I''d love to be able to see patches for each bug separately, but I don''t > think it is possible currently. Do you guys commit the changes in > smaller chunks at Sun and export them to mercurial in bigger chunks, or > this is bascially how you work now? In FreeBSD we always try to keep > unrelated changes in separate commits and this is really useful. > > I think my question is if I can read the changes separately somehow? If > the answer is no, maybe it is worth thinking about improving it a bit in > the future? > > > ------------------------------------------------------------------------ > > _______________________________________________ > zfs-code mailing list > zfs-code at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/zfs-code >
Engineers generally group fixes together because they represent a
single, testable, chunk of overlapping code. Separating them out into
multiple putbacks doesn''t make sense from an engineering or testing
perspective.
That being said, while the putback message contains 20+ bugs, each bug
ID is associated with a smaller subset of files (sometimes just a single
file). With teamware, your only option is to look through the most
recent SCCS revisions of affected files to determine which files were
affected by which bugs. I have no idea how this works in the Mercurial
world, but I remember some discussions on tools-discuss a while ago
about handling multiple bugs in a single changeset.
Part of the problem is that for our internal bug database, we have a
''suggested fix'' field that is the complete fix that particular
bug.
Even if there are a large number of bugs in a single putback, the
suggested fix contains only the changes related to the particular bug
(unless it is truly intertwined with other fixes and can''t be
separated). Unfortunately, this field is not available in the external
bug database. I expect that this will be remedied by moving to an
external DTS (see tools-discuss). In the meantime, here is the
suggested fix for that particular bug which, as you surmised, only
affects vdev_set_state():
------- usr/src/uts/common/fs/zfs/vdev.c -------
Index: usr/src/uts/common/fs/zfs/vdev.c
--- /ws/onnv-clone/usr/src/uts/common/fs/zfs/vdev.c Thu Apr 19 23:09:01 2007
+++ /export/eschrock/zfs-fma/usr/src/uts/common/fs/zfs/vdev.c Wed Jun 6
18:06:52 2007
@@ -1837,12 +1981,44 @@
save_state = vd->vdev_state;
vd->vdev_state = state;
vd->vdev_stat.vs_aux = aux;
- if (state == VDEV_STATE_CANT_OPEN) {
+ /*
+ * If we are setting the vdev state to anything but an open state, then
+ * always close the underlying device. Otherwise, we keep accessible
+ * but invalid devices open forever. We don''t call
vdev_close() itself,
+ * because that implies some extra checks (offline, etc) that we
don''t
+ * want here. This is limited to leaf devices, because otherwise
+ * closing the device will affect other children.
+ */
+ if (vdev_is_dead(vd) && vd->vdev_ops->vdev_op_leaf)
+ vd->vdev_ops->vdev_op_close(vd);
+
*** (#1 of 1): 2007-06-07 16:34:52 PDT eric.schrock at sun.com
I wouldn''t expect developers to stop assembling
''wads'' of change, though
I would expect the tools and processes around them to become more
transparent so it''s easier to track down changes for a particular bug.
You might want to bring this up on tools-discuss, as I''m sure the
Mercurial and DTS folks have been thinking about this particular
problem.
Hope that helps,
- Eric
On Thu, Nov 01, 2007 at 03:51:22PM +0100, Pawel Jakub Dawidek
wrote:> For some time now I''m watching onnv-notify at opensolaris.org.
I''m mostly
> interested in ZFS changes, so I ''hg export
<revision>'' on my downloaded
> repository to take a look. The problem is that it is often really hard
> to tell what the change really does. Let me give an example:
>
> Repository: /hg/onnv/onnv-gate
> Latest revision: 24fbf2d7a5d7d3a91e33eeff0360a238569231bd
> Total changesets: 1
> Log message:
> PSARC 2007/197 ZFS hotplug
> PSARC 2007/283 FMA for ZFS Phase 2
> 6401126 ZFS DE should verify that diagnosis is still valid before solving
cases
> 6500545 ZFS does not handle changes in devids
> 6508521 zpool online should warn when it is being used incorrectly
> 6509807 ZFS checksum ereports are not being posted
> 6514712 zfs_nicenum() doesn''t work with perfectly-sized buffers
> 6520510 media state doesn''t get updated properly on device removal
> 6520513 ZFS should have better support for device removal
> 6520514 vdev state should be controlled through a single ioctl()
> 6520519 ZFS should diagnose faulty devices
> 6520947 ZFS DE should close cases which no longer apply
> 6521393 ZFS case timeout should be FMD_TYPE_TIME
> 6521624 fmd_hash_walk() can dump core when given a bad address
> 6521946 ZFS DE needlessly subscribes to faults
> 6522085 ZFS dictionary files contain spelling errors
> 6523185 vdev_reopen() doesn''t correctly propagate state
> 6523555 ''zpool online'' should be less chatty unless
something goes wrong
> 6527379 zpool(1M) should not try to open faulted devices
> 6527700 ZFS should post a sysevent when topology changes
> 6528194 lofi should support force unmap and DKIO_DEV_GONE
> 6528732 ZFS should store physical device path in addition to /dev path
> 6532635 ZFS keeps devices open unnecessarily
> 6532979 bad argument to ZFS_IOC_VDEV_ATTACH can panic system
> 6567983 deadlock with spa_scrub_thread() and spa_namespace_lock
>
> There is huge number of changes here. Now I''m trying to take only
> "6532635 ZFS keeps devices open unnecessarily" part for inclusion
in
> FreeBSD 7.0-RELEASE. I looked at the patch, I read explanation for
> 6532635, I know part of this change is vdev_set_state() modification,
> but is it the only function modified within this single bug?
>
> Another example. I spent a lot of time trying to track down a lock leak.
> This was a lock leak in zap_micro.c. After I found it, I realized that
> it is already fixed in OpenSolaris:
>
>
http://src.opensolaris.org/source/diff/onnv/onnv-gate/usr/src/uts/common/fs/zfs/zap_micro.c?r2=4831&r1=2856
>
> by this change:
>
> Repository: /hg/onnv/onnv-gate
> Latest revision: 41ec732c6d9fc141be2c6cdccf2f6980f93bd061
> Total changesets: 1
> Log message:
> 6584470 zdb needs to initialize the bpl_lock mutex
> 6583739 libzpool should check for properly initialized mutexes
> 6548010 unbalanced mutex_init/mutex_destroy issues in zfs
> 6502263 ZFS needs some more FreeBSD porting love
> Contributed by Pawel Dawidek
> 6576827 multiple calls to spa_activate() can end up reinitializing all its
mutexes
> 6576830 certain spa mutexes and condition variables need some love
>
> I don''t see anything in the log message which can be used as a
hint for
> this fix.
>
> I''d love to be able to see patches for each bug separately, but I
don''t
> think it is possible currently. Do you guys commit the changes in
> smaller chunks at Sun and export them to mercurial in bigger chunks, or
> this is bascially how you work now? In FreeBSD we always try to keep
> unrelated changes in separate commits and this is really useful.
>
> I think my question is if I can read the changes separately somehow? If
> the answer is no, maybe it is worth thinking about improving it a bit in
> the future?
>
> --
> Pawel Jakub Dawidek http://www.wheel.pl
> pjd at FreeBSD.org http://www.FreeBSD.org
> FreeBSD committer Am I Evil? Yes, I Am!
> _______________________________________________
> zfs-code mailing list
> zfs-code at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/zfs-code
--
Eric Schrock, Solaris Kernel Development http://blogs.sun.com/eschrock
Pawel Jakub Dawidek wrote:> For some time now I''m watching onnv-notify at opensolaris.org. I''m mostly > interested in ZFS changes, so I ''hg export <revision>'' on my downloaded > repository to take a look. The problem is that it is often really hard > to tell what the change really does. Let me give an example: > > Repository: /hg/onnv/onnv-gate > Latest revision: 24fbf2d7a5d7d3a91e33eeff0360a238569231bd > Total changesets: 1 > Log message: > PSARC 2007/197 ZFS hotplug > PSARC 2007/283 FMA for ZFS Phase 2 > 6401126 ZFS DE should verify that diagnosis is still valid before solving cases > 6500545 ZFS does not handle changes in devids > 6508521 zpool online should warn when it is being used incorrectly > 6509807 ZFS checksum ereports are not being posted > 6514712 zfs_nicenum() doesn''t work with perfectly-sized buffers > 6520510 media state doesn''t get updated properly on device removal > 6520513 ZFS should have better support for device removal > 6520514 vdev state should be controlled through a single ioctl() > 6520519 ZFS should diagnose faulty devices > 6520947 ZFS DE should close cases which no longer apply > 6521393 ZFS case timeout should be FMD_TYPE_TIME > 6521624 fmd_hash_walk() can dump core when given a bad address > 6521946 ZFS DE needlessly subscribes to faults > 6522085 ZFS dictionary files contain spelling errors > 6523185 vdev_reopen() doesn''t correctly propagate state > 6523555 ''zpool online'' should be less chatty unless something goes wrong > 6527379 zpool(1M) should not try to open faulted devices > 6527700 ZFS should post a sysevent when topology changes > 6528194 lofi should support force unmap and DKIO_DEV_GONE > 6528732 ZFS should store physical device path in addition to /dev path > 6532635 ZFS keeps devices open unnecessarily > 6532979 bad argument to ZFS_IOC_VDEV_ATTACH can panic system > 6567983 deadlock with spa_scrub_thread() and spa_namespace_lock > > There is huge number of changes here. Now I''m trying to take only > "6532635 ZFS keeps devices open unnecessarily" part for inclusion in > FreeBSD 7.0-RELEASE. I looked at the patch, I read explanation for > 6532635, I know part of this change is vdev_set_state() modification, > but is it the only function modified within this single bug? > > Another example. I spent a lot of time trying to track down a lock leak. > This was a lock leak in zap_micro.c. After I found it, I realized that > it is already fixed in OpenSolaris: > > http://src.opensolaris.org/source/diff/onnv/onnv-gate/usr/src/uts/common/fs/zfs/zap_micro.c?r2=4831&r1=2856 > > by this change: > > Repository: /hg/onnv/onnv-gate > Latest revision: 41ec732c6d9fc141be2c6cdccf2f6980f93bd061 > Total changesets: 1 > Log message: > 6584470 zdb needs to initialize the bpl_lock mutex > 6583739 libzpool should check for properly initialized mutexes > 6548010 unbalanced mutex_init/mutex_destroy issues in zfs > 6502263 ZFS needs some more FreeBSD porting love > Contributed by Pawel Dawidek > 6576827 multiple calls to spa_activate() can end up reinitializing all its mutexes > 6576830 certain spa mutexes and condition variables need some love >Pawel, I fixed this as part of your intial contribution for ''6502263 ZFS needs some more FreeBSD porting love''. When I was doing your changes I detected this issue as well and went ahead and fixed it. So although this wasn''t technically in your provided patch, it was your initial changes that led me to detect the problem and fix it. I didn''t file another bug as I thought your bug encompassed the spirit of this work. Sorry if that wasn''t clearer. Thanks again for contributing! - George
On Thu, Nov 01, 2007 at 08:52:08AM -0700, George Wilson wrote:> I fixed this as part of your intial contribution for ''6502263 ZFS needs > some more FreeBSD porting love''. When I was doing your changes I > detected this issue as well and went ahead and fixed it. So although > this wasn''t technically in your provided patch, it was your initial > changes that led me to detect the problem and fix it. I didn''t file > another bug as I thought your bug encompassed the spirit of this work.Guys, please, don''t take me wrong. I''m not trying to pick on anyone. I''m just sharing my experience of porting stuff from OpenSolaris. I''m fully aware this is something very hard to change. This is only a small inconvenience, which doesn''t change the fact that I very like having access to your very clean and nice code in the first place:) I''m trying to do regular integrations from OpenSolaris, so I can work on as small changes as possible. The smaller the change is, the easier for me to understand what''s going on there and integrate it. When few major features are done in one commit, it''s a bit harder, but nothing I cannot handle, it just takes more time.> Sorry if that wasn''t clearer. Thanks again for contributing!Thank you for intergating my contributions, it helped a lot! I really hope that the FreeBSD community will be able to give something major back at some point. -- Pawel Jakub Dawidek http://www.wheel.pl pjd at FreeBSD.org http://www.FreeBSD.org FreeBSD committer Am I Evil? Yes, I Am! -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 187 bytes Desc: not available URL: <http://mail.opensolaris.org/pipermail/zfs-code/attachments/20071102/e4d758d4/attachment.bin>