* Re: [PATCH 2/2] xfs: log all dirty inodes in xfs_fs_sync_fs
2011-12-18 15:50 ` [PATCH 2/2] xfs: log all dirty inodes in xfs_fs_sync_fs Christoph Hellwig
@ 2011-12-18 20:03 ` Sean Thomas Caron
2011-12-18 20:09 ` Christoph Hellwig
2011-12-18 22:17 ` Dave Chinner
2011-12-20 20:08 ` [PATCH 2/2 v2] " Christoph Hellwig
2 siblings, 1 reply; 20+ messages in thread
From: Sean Thomas Caron @ 2011-12-18 20:03 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Paul Anderson, xfs
Thanks, Christoph. I'll try to get a kernel built tomorrow that
incorporates these patches and give it some testing.
Best,
-Sean
Quoting Christoph Hellwig <hch@infradead.org>:
> Since Linux 2.6.36 the writeback code has introduces various measures for
> live lock prevention during sync(). Unfortunately some of these are
> actively harmful for the XFS model, where the inode gets marked dirty for
> metadata from the data I/O handler.
>
> The older_than_this checks that are now more strictly enforced since
>
> writeback: avoid livelocking WB_SYNC_ALL writeback
>
> by only calling into __writeback_inodes_sb and thus only sampling the
> current cut off time once. But on a slow enough devices the previous
> asynchronous sync pass might not have fully completed yet, and thus XFS
> might mark metadata dirty only after that sampling of the cut off time for
> the blocking pass already happened. I have not myself reproduced this
> myself on a real system, but by introducing artificial delay into the
> XFS I/O completion workqueues it can be reproduced easily.
>
> Fix this by iterating over all XFS inodes in ->sync_fs and log all that
> are dirty. This might log inode that only got redirtied after the
> previous pass, but given how cheap delayed logging of inodes is it
> isn't a major concern for performance.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> Index: xfs/fs/xfs/xfs_sync.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_sync.c 2011-12-14 05:33:06.436599621 -0800
> +++ xfs/fs/xfs/xfs_sync.c 2011-12-14 05:38:49.084743337 -0800
> @@ -336,6 +336,29 @@ xfs_sync_fsdata(
> return error;
> }
>
> +int
> +xfs_log_inode(
> + struct xfs_inode *ip,
> + struct xfs_perag *pag,
> + int flags)
> +{
> + struct xfs_mount *mp = ip->i_mount;
> + struct xfs_trans *tp;
> + int error;
> +
> + tp = xfs_trans_alloc(mp, XFS_TRANS_FSYNC_TS);
> + error = xfs_trans_reserve(tp, 0, XFS_FSYNC_TS_LOG_RES(mp), 0, 0, 0);
> + if (error) {
> + xfs_trans_cancel(tp, 0);
> + return error;
> + }
> +
> + xfs_ilock(ip, XFS_ILOCK_EXCL);
> + xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> + xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> + return xfs_trans_commit(tp, 0);
> +}
> +
> /*
> * When remounting a filesystem read-only or freezing the
> filesystem, we have
> * two phases to execute. This first phase is syncing the data before we
> @@ -359,6 +382,16 @@ xfs_quiesce_data(
> {
> int error, error2 = 0;
>
> + /*
> + * Log all pending size and timestamp updates. The vfs writeback
> + * code is supposed to do this, but due to its overagressive
> + * livelock detection it will skip inodes where appending writes
> + * were written out in the first non-blocking sync phase if their
> + * completion took long enough that it happened after taking the
> + * timestamp for the cut-off in the blocking phase.
> + */
> + xfs_inode_ag_iterator(mp, xfs_log_inode, 0);
> +
> xfs_qm_sync(mp, SYNC_TRYLOCK);
> xfs_qm_sync(mp, SYNC_WAIT);
>
> Index: xfs/fs/xfs/xfs_sync.h
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_sync.h 2011-10-17 00:28:57.255149593 -0700
> +++ xfs/fs/xfs/xfs_sync.h 2011-12-14 05:39:23.187891918 -0800
> @@ -34,6 +34,8 @@ void xfs_quiesce_attr(struct xfs_mount *
>
> void xfs_flush_inodes(struct xfs_inode *ip);
>
> +int xfs_log_inode(struct xfs_inode *ip, struct xfs_perag *pag, int flags);
> +
> int xfs_reclaim_inodes(struct xfs_mount *mp, int mode);
> int xfs_reclaim_inodes_count(struct xfs_mount *mp);
> void xfs_reclaim_inodes_nr(struct xfs_mount *mp, int nr_to_scan);
> Index: xfs/fs/xfs/xfs_super.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_super.c 2011-12-14 05:33:07.193262189 -0800
> +++ xfs/fs/xfs/xfs_super.c 2011-12-14 05:38:56.108038623 -0800
> @@ -869,27 +869,6 @@ xfs_fs_dirty_inode(
> }
>
> STATIC int
> -xfs_log_inode(
> - struct xfs_inode *ip)
> -{
> - struct xfs_mount *mp = ip->i_mount;
> - struct xfs_trans *tp;
> - int error;
> -
> - tp = xfs_trans_alloc(mp, XFS_TRANS_FSYNC_TS);
> - error = xfs_trans_reserve(tp, 0, XFS_FSYNC_TS_LOG_RES(mp), 0, 0, 0);
> - if (error) {
> - xfs_trans_cancel(tp, 0);
> - return error;
> - }
> -
> - xfs_ilock(ip, XFS_ILOCK_EXCL);
> - xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> - xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> - return xfs_trans_commit(tp, 0);
> -}
> -
> -STATIC int
> xfs_fs_write_inode(
> struct inode *inode,
> struct writeback_control *wbc)
> @@ -913,7 +892,7 @@ xfs_fs_write_inode(
> * ->sync_fs call do that for thus, which reduces the number
> * of synchronous log forces dramatically.
> */
> - error = xfs_log_inode(ip);
> + error = xfs_log_inode(ip, NULL, 0);
> if (error)
> goto out;
> return 0;
>
>
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/2] xfs: log all dirty inodes in xfs_fs_sync_fs
2011-12-18 15:50 ` [PATCH 2/2] xfs: log all dirty inodes in xfs_fs_sync_fs Christoph Hellwig
2011-12-18 20:03 ` Sean Thomas Caron
@ 2011-12-18 22:17 ` Dave Chinner
2011-12-18 22:32 ` Christoph Hellwig
2011-12-20 20:08 ` [PATCH 2/2 v2] " Christoph Hellwig
2 siblings, 1 reply; 20+ messages in thread
From: Dave Chinner @ 2011-12-18 22:17 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Paul Anderson, Sean Thomas Caron, xfs
On Sun, Dec 18, 2011 at 10:50:15AM -0500, Christoph Hellwig wrote:
> Since Linux 2.6.36 the writeback code has introduces various measures for
> live lock prevention during sync(). Unfortunately some of these are
> actively harmful for the XFS model, where the inode gets marked dirty for
> metadata from the data I/O handler.
>
> The older_than_this checks that are now more strictly enforced since
>
> writeback: avoid livelocking WB_SYNC_ALL writeback
>
> by only calling into __writeback_inodes_sb and thus only sampling the
> current cut off time once. But on a slow enough devices the previous
> asynchronous sync pass might not have fully completed yet, and thus XFS
> might mark metadata dirty only after that sampling of the cut off time for
> the blocking pass already happened. I have not myself reproduced this
> myself on a real system, but by introducing artificial delay into the
> XFS I/O completion workqueues it can be reproduced easily.
>
> Fix this by iterating over all XFS inodes in ->sync_fs and log all that
> are dirty. This might log inode that only got redirtied after the
> previous pass, but given how cheap delayed logging of inodes is it
> isn't a major concern for performance.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> Index: xfs/fs/xfs/xfs_sync.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_sync.c 2011-12-14 05:33:06.436599621 -0800
> +++ xfs/fs/xfs/xfs_sync.c 2011-12-14 05:38:49.084743337 -0800
> @@ -336,6 +336,29 @@ xfs_sync_fsdata(
> return error;
> }
>
> +int
> +xfs_log_inode(
> + struct xfs_inode *ip,
> + struct xfs_perag *pag,
> + int flags)
> +{
> + struct xfs_mount *mp = ip->i_mount;
> + struct xfs_trans *tp;
> + int error;
> +
> + tp = xfs_trans_alloc(mp, XFS_TRANS_FSYNC_TS);
> + error = xfs_trans_reserve(tp, 0, XFS_FSYNC_TS_LOG_RES(mp), 0, 0, 0);
> + if (error) {
> + xfs_trans_cancel(tp, 0);
> + return error;
> + }
> +
> + xfs_ilock(ip, XFS_ILOCK_EXCL);
> + xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> + xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> + return xfs_trans_commit(tp, 0);
> +}
This will do a transaction on the inode, clean or dirty. That's an
awful lot of overhead for the few inodes (out of perhaps millions in
memory) that actually need it. with the ->dirty_inode callback from
the VFS, we know the only inodes that need logging are those with
i_update_core set....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/2] xfs: log all dirty inodes in xfs_fs_sync_fs
2011-12-18 22:17 ` Dave Chinner
@ 2011-12-18 22:32 ` Christoph Hellwig
0 siblings, 0 replies; 20+ messages in thread
From: Christoph Hellwig @ 2011-12-18 22:32 UTC (permalink / raw)
To: Dave Chinner; +Cc: Christoph Hellwig, Paul Anderson, Sean Thomas Caron, xfs
On Mon, Dec 19, 2011 at 09:17:07AM +1100, Dave Chinner wrote:
> This will do a transaction on the inode, clean or dirty. That's an
> awful lot of overhead for the few inodes (out of perhaps millions in
> memory) that actually need it. with the ->dirty_inode callback from
> the VFS, we know the only inodes that need logging are those with
> i_update_core set....
Ooops, I messed that up when forwarding the RFC patch I sent to
Paul & Sean, and that I had been testing with most of the time. The
original one moved the i_update_core check into xfs_log_inode and that
is how it should be done. I'll resend it.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 2/2 v2] xfs: log all dirty inodes in xfs_fs_sync_fs
2011-12-18 15:50 ` [PATCH 2/2] xfs: log all dirty inodes in xfs_fs_sync_fs Christoph Hellwig
2011-12-18 20:03 ` Sean Thomas Caron
2011-12-18 22:17 ` Dave Chinner
@ 2011-12-20 20:08 ` Christoph Hellwig
2011-12-20 21:21 ` Dave Chinner
` (2 more replies)
2 siblings, 3 replies; 20+ messages in thread
From: Christoph Hellwig @ 2011-12-20 20:08 UTC (permalink / raw)
To: xfs; +Cc: Paul Anderson, Sean Thomas Caron
Since Linux 2.6.36 the writeback code has introduces various measures for
live lock prevention during sync(). Unfortunately some of these are
actively harmful for the XFS model, where the inode gets marked dirty for
metadata from the data I/O handler.
The older_than_this checks that are now more strictly enforced since
writeback: avoid livelocking WB_SYNC_ALL writeback
by only calling into __writeback_inodes_sb and thus only sampling the
current cut off time once. But on a slow enough devices the previous
asynchronous sync pass might not have fully completed yet, and thus XFS
might mark metadata dirty only after that sampling of the cut off time for
the blocking pass already happened. I have not myself reproduced this
myself on a real system, but by introducing artificial delay into the
XFS I/O completion workqueues it can be reproduced easily.
Fix this by iterating over all XFS inodes in ->sync_fs and log all that
are dirty. This might log inode that only got redirtied after the
previous pass, but given how cheap delayed logging of inodes is it
isn't a major concern for performance.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Index: xfs/fs/xfs/xfs_sync.c
===================================================================
--- xfs.orig/fs/xfs/xfs_sync.c 2011-12-18 23:35:30.909834592 +0100
+++ xfs/fs/xfs/xfs_sync.c 2011-12-18 23:36:40.823167508 +0100
@@ -336,6 +336,32 @@ xfs_sync_fsdata(
return error;
}
+int
+xfs_log_dirty_inode(
+ struct xfs_inode *ip,
+ struct xfs_perag *pag,
+ int flags)
+{
+ struct xfs_mount *mp = ip->i_mount;
+ struct xfs_trans *tp;
+ int error;
+
+ if (!ip->i_update_core)
+ return 0;
+
+ tp = xfs_trans_alloc(mp, XFS_TRANS_FSYNC_TS);
+ error = xfs_trans_reserve(tp, 0, XFS_FSYNC_TS_LOG_RES(mp), 0, 0, 0);
+ if (error) {
+ xfs_trans_cancel(tp, 0);
+ return error;
+ }
+
+ xfs_ilock(ip, XFS_ILOCK_EXCL);
+ xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
+ xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
+ return xfs_trans_commit(tp, 0);
+}
+
/*
* When remounting a filesystem read-only or freezing the filesystem, we have
* two phases to execute. This first phase is syncing the data before we
@@ -359,6 +385,16 @@ xfs_quiesce_data(
{
int error, error2 = 0;
+ /*
+ * Log all pending size and timestamp updates. The vfs writeback
+ * code is supposed to do this, but due to its overagressive
+ * livelock detection it will skip inodes where appending writes
+ * were written out in the first non-blocking sync phase if their
+ * completion took long enough that it happened after taking the
+ * timestamp for the cut-off in the blocking phase.
+ */
+ xfs_inode_ag_iterator(mp, xfs_log_dirty_inode, 0);
+
xfs_qm_sync(mp, SYNC_TRYLOCK);
xfs_qm_sync(mp, SYNC_WAIT);
Index: xfs/fs/xfs/xfs_sync.h
===================================================================
--- xfs.orig/fs/xfs/xfs_sync.h 2011-12-18 16:47:25.000000000 +0100
+++ xfs/fs/xfs/xfs_sync.h 2011-12-18 23:36:45.119834149 +0100
@@ -34,6 +34,8 @@ void xfs_quiesce_attr(struct xfs_mount *
void xfs_flush_inodes(struct xfs_inode *ip);
+int xfs_log_dirty_inode(struct xfs_inode *ip, struct xfs_perag *pag, int flags);
+
int xfs_reclaim_inodes(struct xfs_mount *mp, int mode);
int xfs_reclaim_inodes_count(struct xfs_mount *mp);
void xfs_reclaim_inodes_nr(struct xfs_mount *mp, int nr_to_scan);
Index: xfs/fs/xfs/xfs_super.c
===================================================================
--- xfs.orig/fs/xfs/xfs_super.c 2011-12-18 23:35:42.863167854 +0100
+++ xfs/fs/xfs/xfs_super.c 2011-12-18 23:36:42.963167495 +0100
@@ -869,27 +869,6 @@ xfs_fs_dirty_inode(
}
STATIC int
-xfs_log_inode(
- struct xfs_inode *ip)
-{
- struct xfs_mount *mp = ip->i_mount;
- struct xfs_trans *tp;
- int error;
-
- tp = xfs_trans_alloc(mp, XFS_TRANS_FSYNC_TS);
- error = xfs_trans_reserve(tp, 0, XFS_FSYNC_TS_LOG_RES(mp), 0, 0, 0);
- if (error) {
- xfs_trans_cancel(tp, 0);
- return error;
- }
-
- xfs_ilock(ip, XFS_ILOCK_EXCL);
- xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
- xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
- return xfs_trans_commit(tp, 0);
-}
-
-STATIC int
xfs_fs_write_inode(
struct inode *inode,
struct writeback_control *wbc)
@@ -902,8 +881,6 @@ xfs_fs_write_inode(
if (XFS_FORCED_SHUTDOWN(mp))
return -XFS_ERROR(EIO);
- if (!ip->i_update_core)
- return 0;
if (wbc->sync_mode == WB_SYNC_ALL || wbc->for_kupdate) {
/*
@@ -913,11 +890,14 @@ xfs_fs_write_inode(
* ->sync_fs call do that for thus, which reduces the number
* of synchronous log forces dramatically.
*/
- error = xfs_log_inode(ip);
+ error = xfs_log_dirty_inode(ip, NULL, 0);
if (error)
goto out;
return 0;
} else {
+ if (!ip->i_update_core)
+ return 0;
+
/*
* We make this non-blocking if the inode is contended, return
* EAGAIN to indicate to the caller that they did not succeed.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/2 v2] xfs: log all dirty inodes in xfs_fs_sync_fs
2011-12-20 20:08 ` [PATCH 2/2 v2] " Christoph Hellwig
@ 2011-12-20 21:21 ` Dave Chinner
2011-12-23 15:55 ` Mark Tinguely
2011-12-23 21:47 ` Ben Myers
2 siblings, 0 replies; 20+ messages in thread
From: Dave Chinner @ 2011-12-20 21:21 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Paul Anderson, Sean Thomas Caron, xfs
On Tue, Dec 20, 2011 at 03:08:41PM -0500, Christoph Hellwig wrote:
> Since Linux 2.6.36 the writeback code has introduces various measures for
> live lock prevention during sync(). Unfortunately some of these are
> actively harmful for the XFS model, where the inode gets marked dirty for
> metadata from the data I/O handler.
>
> The older_than_this checks that are now more strictly enforced since
>
> writeback: avoid livelocking WB_SYNC_ALL writeback
>
> by only calling into __writeback_inodes_sb and thus only sampling the
> current cut off time once. But on a slow enough devices the previous
> asynchronous sync pass might not have fully completed yet, and thus XFS
> might mark metadata dirty only after that sampling of the cut off time for
> the blocking pass already happened. I have not myself reproduced this
> myself on a real system, but by introducing artificial delay into the
> XFS I/O completion workqueues it can be reproduced easily.
>
> Fix this by iterating over all XFS inodes in ->sync_fs and log all that
> are dirty. This might log inode that only got redirtied after the
> previous pass, but given how cheap delayed logging of inodes is it
> isn't a major concern for performance.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Looks OK now.
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2 v2] xfs: log all dirty inodes in xfs_fs_sync_fs
2011-12-20 20:08 ` [PATCH 2/2 v2] " Christoph Hellwig
2011-12-20 21:21 ` Dave Chinner
@ 2011-12-23 15:55 ` Mark Tinguely
2011-12-23 21:47 ` Ben Myers
2 siblings, 0 replies; 20+ messages in thread
From: Mark Tinguely @ 2011-12-23 15:55 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Paul Anderson, Sean Thomas Caron, xfs
Tested-by: Mark Tinguely <tinguely@sgi.com>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2 v2] xfs: log all dirty inodes in xfs_fs_sync_fs
2011-12-20 20:08 ` [PATCH 2/2 v2] " Christoph Hellwig
2011-12-20 21:21 ` Dave Chinner
2011-12-23 15:55 ` Mark Tinguely
@ 2011-12-23 21:47 ` Ben Myers
2011-12-26 12:13 ` Dave Chinner
2 siblings, 1 reply; 20+ messages in thread
From: Ben Myers @ 2011-12-23 21:47 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Paul Anderson, Sean Thomas Caron, Mark Tinguely, xfs
Hey Christoph,
On Tue, Dec 20, 2011 at 03:08:41PM -0500, Christoph Hellwig wrote:
> Since Linux 2.6.36 the writeback code has introduces various measures for
> live lock prevention during sync(). Unfortunately some of these are
> actively harmful for the XFS model, where the inode gets marked dirty for
> metadata from the data I/O handler.
>
> The older_than_this checks that are now more strictly enforced since
>
> writeback: avoid livelocking WB_SYNC_ALL writeback
>
> by only calling into __writeback_inodes_sb and thus only sampling the
Do you mean __writeback_inodes_wb, or perhaps writeback_sb_inodes?
So far I'm not seeing the connection with the commit you've referenced
above, which seems to be related to nr_to_write. It looks like you're
referring to older fs/fs-writeback.c..
This one seems like it might be relevant:
commit 7624ee7
mm: avoid resetting wb_start after each writeback round
Anyway.. in the 3.2-rcX code it appears that older_than_this is only set
at the start of wb_writeback (excluding for_kupdate being set)...
> current cut off time once. But on a slow enough devices the previous
> asynchronous sync pass might not have fully completed yet, and thus XFS
> might mark metadata dirty only after that sampling of the cut off time for
> the blocking pass already happened.
Are you referring to sync_filesystem() calling
__sync_filesystem(sb, 0)
and then
__sync_filesystem(sb, 1 /* wait */)?
Ah... and with that I think I understand what you're after here: After
__sync_filesystem calls sync_inodes_sb and waits for completion... which
will dirty more inodes in completion handlers... you have the
opportunity to clean up those dirty inodes in .sync_fs. If that's what
you intend: I'd say this looks good.
Reviewed-by: Ben Myers <bpm@sgi.com>
Mark also reviewed this.
Reviewed-by: Mark Tinguely <tinguely@sgi.com>
THanks,
Ben
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2 v2] xfs: log all dirty inodes in xfs_fs_sync_fs
2011-12-23 21:47 ` Ben Myers
@ 2011-12-26 12:13 ` Dave Chinner
2011-12-29 15:42 ` Ben Myers
0 siblings, 1 reply; 20+ messages in thread
From: Dave Chinner @ 2011-12-26 12:13 UTC (permalink / raw)
To: Ben Myers
Cc: Christoph Hellwig, Paul Anderson, Sean Thomas Caron,
Mark Tinguely, xfs
On Fri, Dec 23, 2011 at 03:47:03PM -0600, Ben Myers wrote:
>
> Reviewed-by: Ben Myers <bpm@sgi.com>
>
> Mark also reviewed this.
>
> Reviewed-by: Mark Tinguely <tinguely@sgi.com>
Just a process note here: if Mark reviewed the code and is happy
with it, then he needs to send his reviewed-by tag himself. If he's
got concerns, then he needs to discuss them on the list with the
patch author, not just in private with you. If a person's questions
are not posted to the mailing list or posted by proxy and they
didn't aprticipate in discussions on the list, then there is no
evidence that the person ever reviewed the patch. Hence the tag has
no value because it is not verifiable.
More importantly, tags are a semi-formal statement that a set of
actions has been taken by that person - see
Documentation/SubmittingPatches for the actions different tags
imply. Hence it is important the actions they imply are verifiable,
and it also reinforces the fact that they only have value when they
are issued by the email address (or a known alias) in the tag....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2 v2] xfs: log all dirty inodes in xfs_fs_sync_fs
2011-12-26 12:13 ` Dave Chinner
@ 2011-12-29 15:42 ` Ben Myers
2011-12-29 21:44 ` Dave Chinner
0 siblings, 1 reply; 20+ messages in thread
From: Ben Myers @ 2011-12-29 15:42 UTC (permalink / raw)
To: Dave Chinner
Cc: Christoph Hellwig, Paul Anderson, Sean Thomas Caron,
Mark Tinguely, xfs
Hi Dave,
On Mon, Dec 26, 2011 at 11:13:02PM +1100, Dave Chinner wrote:
> On Fri, Dec 23, 2011 at 03:47:03PM -0600, Ben Myers wrote:
> >
> > Reviewed-by: Ben Myers <bpm@sgi.com>
> >
> > Mark also reviewed this.
> >
> > Reviewed-by: Mark Tinguely <tinguely@sgi.com>
>
> Just a process note here: if Mark reviewed the code and is happy
> with it, then he needs to send his reviewed-by tag himself. If he's
> got concerns, then he needs to discuss them on the list with the
> patch author, not just in private with you. If a person's questions
> are not posted to the mailing list or posted by proxy and they
> didn't aprticipate in discussions on the list, then there is no
> evidence that the person ever reviewed the patch. Hence the tag has
> no value because it is not verifiable.
I tend to agree that it is important to discuss things openly on the
list. Will make an effort to do more of this.
> More importantly, tags are a semi-formal statement that a set of
> actions has been taken by that person - see
> Documentation/SubmittingPatches for the actions different tags
> imply. Hence it is important the actions they imply are verifiable,
> and it also reinforces the fact that they only have value when they
> are issued by the email address (or a known alias) in the tag....
I don't see anything in SubmittingPatches that says the address on the
>From line not matching a tag is a dealbreaker, and I think that we
should give credit where it is due. Mark did some work to review and
understand this code in addition to his testing. I have him a call and
asked him if I could add a 'Reviewed-by' to his 'Tested-by' because I
was suprised he didn't... Next time I'll ask him to send it himself.
I'd like to point out that plenty of the conversation surrounding this
pair of patches seems not to have made it to the list either.
Anyway... SGI folk will try to keep discussion on the list or on irc
next time.
Folks from umich: if you offer a 'Tested-by' we'll add that too.
Thanks,
Ben
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2 v2] xfs: log all dirty inodes in xfs_fs_sync_fs
2011-12-29 15:42 ` Ben Myers
@ 2011-12-29 21:44 ` Dave Chinner
2012-01-03 15:48 ` Mark Tinguely
0 siblings, 1 reply; 20+ messages in thread
From: Dave Chinner @ 2011-12-29 21:44 UTC (permalink / raw)
To: Ben Myers
Cc: Christoph Hellwig, Paul Anderson, Sean Thomas Caron,
Mark Tinguely, xfs
On Thu, Dec 29, 2011 at 09:42:07AM -0600, Ben Myers wrote:
> Hi Dave,
>
> On Mon, Dec 26, 2011 at 11:13:02PM +1100, Dave Chinner wrote:
> > On Fri, Dec 23, 2011 at 03:47:03PM -0600, Ben Myers wrote:
> > >
> > > Reviewed-by: Ben Myers <bpm@sgi.com>
> > >
> > > Mark also reviewed this.
> > >
> > > Reviewed-by: Mark Tinguely <tinguely@sgi.com>
> >
> > Just a process note here: if Mark reviewed the code and is happy
> > with it, then he needs to send his reviewed-by tag himself. If he's
> > got concerns, then he needs to discuss them on the list with the
> > patch author, not just in private with you. If a person's questions
> > are not posted to the mailing list or posted by proxy and they
> > didn't aprticipate in discussions on the list, then there is no
> > evidence that the person ever reviewed the patch. Hence the tag has
> > no value because it is not verifiable.
>
> I tend to agree that it is important to discuss things openly on the
> list. Will make an effort to do more of this.
>
> > More importantly, tags are a semi-formal statement that a set of
> > actions has been taken by that person - see
> > Documentation/SubmittingPatches for the actions different tags
> > imply. Hence it is important the actions they imply are verifiable,
> > and it also reinforces the fact that they only have value when they
> > are issued by the email address (or a known alias) in the tag....
>
> I don't see anything in SubmittingPatches that says the address on the
> From line not matching a tag is a dealbreaker, and I think that we
> should give credit where it is due. Mark did some work to review and
> understand this code in addition to his testing.
But credit is not what the Reviewed-by tag means - it's a statement
of fact about actions performed by the reviewer. A partial review or
partaking in part of a review does not mean a person can put a
"reviewed-by" tag on a commit. An Acked-by my be appropriate in that
case (though I see them a worthless by their very nature), but
reviewed-by tags are not for "giving credit" to other people that
may have helped you.
Further, keep in mind that I've only ever seen 2 emails from Mark,
so I have no idea who he is or what his capabilities are, so I do
not yet know how much to trust his reviews or testing. Until I've
spend some time interacting with him directly, I won't be able to
form those opinions, and so such tags start at the lower end of
value. They have even less value when they don't come from him and
he hasn't commented where I can see it.
> I have him a call and
> asked him if I could add a 'Reviewed-by' to his 'Tested-by' because I
> was suprised he didn't... Next time I'll ask him to send it himself.
So, perhaps he didn't consider what he did met all the criteria of a
reviewed-by tag? See what I mean about tags being worthless when sent
by proxy?
> I'd like to point out that plenty of the conversation surrounding this
> pair of patches seems not to have made it to the list either.
Happens all the time, especially on #xfs. The difference is, though,
that such conversations are not "reviews" or result in one person
sending reviewed-by tags for all the participants in the
conversation....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2 v2] xfs: log all dirty inodes in xfs_fs_sync_fs
2011-12-29 21:44 ` Dave Chinner
@ 2012-01-03 15:48 ` Mark Tinguely
0 siblings, 0 replies; 20+ messages in thread
From: Mark Tinguely @ 2012-01-03 15:48 UTC (permalink / raw)
To: Dave Chinner
Cc: Christoph Hellwig, Paul Anderson, Ben Myers, Sean Thomas Caron,
xfs
On 12/29/11 15:44, Dave Chinner wrote:
> On Thu, Dec 29, 2011 at 09:42:07AM -0600, Ben Myers wrote:
>> Hi Dave,
>>
>> On Mon, Dec 26, 2011 at 11:13:02PM +1100, Dave Chinner wrote:
>>> On Fri, Dec 23, 2011 at 03:47:03PM -0600, Ben Myers wrote:
>>>>
>>>> Reviewed-by: Ben Myers<bpm@sgi.com>
>>>>
>>>> Mark also reviewed this.
>>>>
>>>> Reviewed-by: Mark Tinguely<tinguely@sgi.com>
>>>
>>> Just a process note here: if Mark reviewed the code and is happy
>>> with it, then he needs to send his reviewed-by tag himself. If he's
>>> got concerns, then he needs to discuss them on the list with the
>>> patch author, not just in private with you. If a person's questions
>>> are not posted to the mailing list or posted by proxy and they
>>> didn't aprticipate in discussions on the list, then there is no
>>> evidence that the person ever reviewed the patch. Hence the tag has
>>> no value because it is not verifiable.
>>
>> I tend to agree that it is important to discuss things openly on the
>> list. Will make an effort to do more of this.
>>
>>> More importantly, tags are a semi-formal statement that a set of
>>> actions has been taken by that person - see
>>> Documentation/SubmittingPatches for the actions different tags
>>> imply. Hence it is important the actions they imply are verifiable,
>>> and it also reinforces the fact that they only have value when they
>>> are issued by the email address (or a known alias) in the tag....
>>
>> I don't see anything in SubmittingPatches that says the address on the
>> From line not matching a tag is a dealbreaker, and I think that we
>> should give credit where it is due. Mark did some work to review and
>> understand this code in addition to his testing.
>
> But credit is not what the Reviewed-by tag means - it's a statement
> of fact about actions performed by the reviewer. A partial review or
> partaking in part of a review does not mean a person can put a
> "reviewed-by" tag on a commit. An Acked-by my be appropriate in that
> case (though I see them a worthless by their very nature), but
> reviewed-by tags are not for "giving credit" to other people that
> may have helped you.
>
> Further, keep in mind that I've only ever seen 2 emails from Mark,
> so I have no idea who he is or what his capabilities are, so I do
> not yet know how much to trust his reviews or testing. Until I've
> spend some time interacting with him directly, I won't be able to
> form those opinions, and so such tags start at the lower end of
> value. They have even less value when they don't come from him and
> he hasn't commented where I can see it.
>
>> I have him a call and
>> asked him if I could add a 'Reviewed-by' to his 'Tested-by' because I
>> was suprised he didn't... Next time I'll ask him to send it himself.
>
> So, perhaps he didn't consider what he did met all the criteria of a
> reviewed-by tag? See what I mean about tags being worthless when sent
> by proxy?
>
>> I'd like to point out that plenty of the conversation surrounding this
>> pair of patches seems not to have made it to the list either.
>
> Happens all the time, especially on #xfs. The difference is, though,
> that such conversations are not "reviews" or result in one person
> sending reviewed-by tags for all the participants in the
> conversation....
>
> Cheers,
>
> Dave.
Thank-you for the feedback. There are several good points made in this
thread.
--Mark Tinguely.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 20+ messages in thread