Refactor generic inode time updating #4916

lorddoskias · 2016-08-01T20:07:13Z

So here is my initial go at moving more code away from zfs_inode_update_impl. This shouldn't cause any visible behavior changes.

tuxoko · 2016-08-01T22:11:32Z

@lorddoskias
Every file operations that update mtime, ctime will update SA and then call zpl_inode_update to update time in inode. So what you do here will make mtime, ctime only update in SA. What you can do is to come up with a new api for updating mtime, ctime to handle this.

Edit: see zfs_tstamp_update_setup

behlendorf · 2016-08-02T21:25:27Z

Extending zfs_tstamp_update_setup() to also update the inode may be a reasonable way to do this. But you'll want to audit all the locations where mtime and ctime are updated.

lorddoskias · 2016-08-09T19:48:15Z

Provided the tests for those 3 commits succeed I'd like to add another one which simplifies the zfs_inode_update_impl byt removing the code necessary to support the "new" variant. So don't merge until then.

lorddoskias · 2016-08-09T20:08:47Z

module/zfs/zpl_file.c

@@ -361,8 +361,10 @@ zpl_write(struct file *filp, const char __user *buf, size_t len, loff_t *ppos)
 	ssize_t wrote;

 	crhold(cr);
+	spl_inode_lock(file_inode(filp));
 	wrote = zpl_write_common(filp->f_mapping->host, buf, len, ppos,


@tuxoko @behlendorf Is there any reason we are getting the inode from f_mapping->host and not directly from file_inode? That'd be one dereference and make the code more succinct and clear. In my testing I put an assert for f_inode == f_mapping_host and it never fired, so I assume it's safe?

Backward compat.

Edit: Oh, we actually already have file_inode compat layer.

Indee, and even in the compat layer the inode ref is taken via the dentry and not the f_mapping. Though it shouldn't really matter. So are you happy with me changing the argument to zpl_write_common to utilise file_inode?

Originally it was for backwards compatibility but since this code was originally written we've added a file_inode() compat wrapper to support all the way back to 2.6.32 kernels (which is the oldest kernel we support). You should be able to safely change this to file_inode().

We could still use a file_dentry() compat wrapper, see #4935.

So are you happy with me changing the argument to zpl_write_common to utilise file_inode?

Yup.

lorddoskias · 2016-08-11T14:38:39Z

Did this fail due to a bug (deadlock) or due to amazon problems?

behlendorf · 2016-08-11T20:36:42Z

@lorddoskias Amazon problems which have been resolved (I hope). Can you rebase this on master. The patch to add file_dentry() has been merged as well as the last of the Linux 4.8 compat fixes. That means everything should pass with the exception of the one-off test failures.

edit: I lied, this failed almost certainly due to commit c484597. We shouldn't need to make this change at all.

behlendorf · 2016-08-11T20:53:12Z

module/zfs/zfs_znode.c

-	 * Only read atime from SA if we are newly created inode (or rezget),
-	 * otherwise i_atime might be dirty.
-	 */
-	if (new)


As part of this change you can remove new from the function arguments (which was recently added) and the zfs_inode_update_new() function. Or we can leave it as dead code for now and remove it when this function is completely removed.

Yeah, Intended to do this change and add it do this PR but I wanted to clear the other commits first. So, the reason why I locked the zfs_write is that the inode->i_mtime etc attributes are being changed. And just to be on the safe side I wanted to lock the vfs inode while the operations is in progress, otherwise it might be possible to initiate 2 separate operations which might lead to corruption. E.g. a write + truncate (maybe, I'm only speculating)

The existing range lock will guarantee a consistent ordering as long as you perform the inode updates while it's being held. Which is why zfs_inode_update() always happens before zfs_range_unlock().

behlendorf · 2016-08-15T23:52:45Z

Definite progress, but it appears that ctime_001_pos is failing on all testers because the mtime isn't being set correctly for creat(2). The next time you refresh this you might consider squashing all the commits in to one, I'm not sure there's any benefit keeping them separate.

lorddoskias · 2016-08-16T19:24:39Z

@behlendorf I think I might have found the culprit and it might turned out to be a latent bug in zfs. So when a file is created in a zfs directory here are the situations where the mtime is being updated:

[   40.537646] We are in the second branch : 32
[   40.537989] CPU: 1 PID: 879 Comm: touch Tainted: G           O    4.6.0-nbor #27
[   40.538630] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
[   40.539417]  0000000000000000 ffff8800366c7b70 ffffffff813c3873 ffff8800366c87f8
[   40.540033]  0000000000000040 ffff8800366c7bb0 ffffffffa01f7cff 00000000380c5d4e
[   40.540683]  ffff88003c87dcc0 0000000000000070 0000000000000000 ffff8800366c8c90
[   40.541303] Call Trace:
[   40.541503]  [<ffffffff813c3873>] dump_stack+0x85/0xc2
[   40.541944]  [<ffffffffa01f7cff>] zfs_tstamp_update_setup+0xcf/0x140 [zfs]
[   40.542512]  [<ffffffffa01e7e99>] zfs_setattr+0x1219/0x27c0 [zfs]
[   40.542992]  [<ffffffff8107ba89>] ? __might_sleep+0x49/0x80
[   40.543437]  [<ffffffff8119be68>] ? __kmalloc_node+0x298/0x4b0
[   40.543910]  [<ffffffffa0001154>] ? spl_kmem_zalloc+0xb4/0x1f0 [spl]
[   40.544409]  [<ffffffffa0001154>] ? spl_kmem_zalloc+0xb4/0x1f0 [spl]
[   40.544945]  [<ffffffffa0213190>] zpl_setattr+0x120/0x210 [zfs]
[   40.545410]  [<ffffffff811d5692>] notify_change+0x202/0x350
[   40.545846]  [<ffffffff811ecac4>] utimes_common+0xc4/0x1c0
[   40.546275]  [<ffffffff811d672f>] ? __fget_light+0x6f/0x90
[   40.546705]  [<ffffffff811ecccc>] do_utimes+0x10c/0x130
[   40.547125]  [<ffffffff811ece0e>] SyS_utimensat+0x6e/0xa0
[   40.547562]  [<ffffffff81600440>] entry_SYSCALL_64_fastpath+0x23/0xc1
[   40.548064]  [<ffffffff810a017f>] ? trace_hardirqs_off_caller+0x1f/0xc0
[   40.548614] Old : 918334414
[   40.548880] NEW : 940334414
[root@localhost ~]# stat zfs-mount/ludnica 
  File: 'zfs-mount/ludnica'
  Size: 0           Blocks: 1          IO Block: 131072 regular empty file
Device: 20h/32d Inode: 7           Links: 1
Access: (0644/-rw-r--r--)  Uid: (    0/    root)   Gid: (    0/    root)
Access: 2016-08-16 19:08:00.940334414 +0000
Modify: 2016-08-16 19:08:00.918334414 +0000
Change: 2016-08-16 19:08:00.940334414 +0000
 Birth: -

So looking at the stack trace (triggered by the following debug code zfs_tstamp_update_setup) :

if (flag & ATTR_MTIME) {
        dump_stack();
        pr_info("Old : %ld\n", (ZTOI(zp)->i_mtime.tv_nsec));
        ZFS_TIME_ENCODE(&now, mtime);
        ZFS_TIME_DECODE(&(ZTOI(zp)->i_mtime), mtime);
        pr_info("NEW : %ld\n", (ZTOI(zp)->i_mtime.tv_nsec));
        if (ZTOZSB(zp)->z_use_fuids) {
            zp->z_pflags |= (ZFS_ARCHIVE |
                ZFS_AV_MODIFIED);
        }
    }

    if (flag & ATTR_CTIME) {
        dump_stack();
        pr_info("Old : %ld\n", (ZTOI(zp)->i_ctime.tv_nsec));
        ZFS_TIME_ENCODE(&now, ctime);
        ZFS_TIME_DECODE(&(ZTOI(zp)->i_ctime), ctime);
        pr_info("NEW : %ld\n", (ZTOI(zp)->i_ctime.tv_nsec));
        if (ZTOZSB(zp)->z_use_fuids)
            zp->z_pflags |= ZFS_ARCHIVE;
    }

As can be seen only the second branch is being triggered, hence the mtime is not being updated. Again looking at the initial backtrace the tstamp_update function is called from zfs_setattr, which has the following code:

  if (mask & ATTR_SIZE && !(mask & ATTR_MTIME)) {
        pr_info ("We are in the first branch where mtime/ctime is updated\n");
        SA_ADD_BULK_ATTR(bulk, count, SA_ZPL_MTIME(zsb),
            NULL, &mtime, sizeof (mtime));
        SA_ADD_BULK_ATTR(bulk, count, SA_ZPL_CTIME(zsb), NULL,
            &ctime, sizeof (ctime));
        zfs_tstamp_update_setup(zp, CONTENT_MODIFIED, mtime, ctime);
    } else if (mask != 0) {
        pr_info ("We are in the second branch : %d\n", (mask & ATTR_MTIME));
        SA_ADD_BULK_ATTR(bulk, count, SA_ZPL_CTIME(zsb), NULL,
            &ctime, sizeof (ctime));
        zfs_tstamp_update_setup(zp, STATE_CHANGED, mtime, ctime);
......
    }

So in this case we are in the second branch, where tstamp_update is called with STATE_CHANGED, which is defined to be CTIME. However, in the else clause we are only updating the ctime, and completely ignoring whether ATTR_MTIME is actually being set. And in this case ATTR_MTIME is in fact set.

The following patch seems to fix the issue, does it look correct to you:

diff --git a/module/zfs/zfs_vnops.c b/module/zfs/zfs_vnops.c
index 707a21114943..2a6880dad4cd 100644
--- a/module/zfs/zfs_vnops.c
+++ b/module/zfs/zfs_vnops.c
@@ -3016,16 +3016,20 @@ top:

        /* XXX - shouldn't this be done *before* the ATIME/MTIME checks? */
        if (mask & ATTR_SIZE && !(mask & ATTR_MTIME)) {
                SA_ADD_BULK_ATTR(bulk, count, SA_ZPL_MTIME(zsb),
                   NULL, mtime, sizeof (mtime));
                SA_ADD_BULK_ATTR(bulk, count, SA_ZPL_CTIME(zsb), NULL,
                    &ctime, sizeof (ctime));
                zfs_tstamp_update_setup(zp, CONTENT_MODIFIED, mtime, ctime);
        } else if (mask != 0) {
+               uint32_t change_type = (mask & ATTR_MTIME) ? CONTENT_MODIFIED : STATE_CHANGED;
                SA_ADD_BULK_ATTR(bulk, count, SA_ZPL_CTIME(zsb), NULL,
                    &ctime, sizeof (ctime));
-               zfs_tstamp_update_setup(zp, STATE_CHANGED, mtime, ctime);
+               zfs_tstamp_update_setup(zp, change_type, mtime, ctime);
                if (attrzp) {
                        SA_ADD_BULK_ATTR(xattr_bulk, xattr_count,
                            SA_ZPL_CTIME(zsb), NULL,
                            &ctime, sizeof (ctime));

behlendorf · 2016-08-16T20:58:18Z

That looks correct to me, this is almost certainly an issue caused by the differences in how illumos and Linux handle these bits. My only suggestion is that something like the following patch may be more readable (untested).

diff --git a/module/zfs/zfs_vnops.c b/module/zfs/zfs_vnops.c
index 707a211..67ecd2f 100644
--- a/module/zfs/zfs_vnops.c
+++ b/module/zfs/zfs_vnops.c
@@ -3024,7 +3024,8 @@ top:
        } else if (mask != 0) {
                SA_ADD_BULK_ATTR(bulk, count, SA_ZPL_CTIME(zsb), NULL,
                    &ctime, sizeof (ctime));
-               zfs_tstamp_update_setup(zp, STATE_CHANGED, mtime, ctime);
+               zfs_tstamp_update_setup(zp, mask & (ATTR_MTIME | ATTR_CTIME),
+                   mtime, ctime);
                if (attrzp) {
                        SA_ADD_BULK_ATTR(xattr_bulk, xattr_count,
                            SA_ZPL_CTIME(zsb), NULL,

pro-tip: Use diff <patch> around the patch to colorize it.

lorddoskias · 2016-08-16T21:27:11Z

@behlendorf You suggestion works and I will use it. I also prefer this change to be in a separate commit for the sake of git history. Also I started looking a bit closer into zfs_setattr and wonder whether all the code dealing with XVATTR should be removed? Clearly this is an artifact from illumos and the zfs_setattr function is very large and hard to follow, let alone reason for correctness?

behlendorf · 2016-08-16T21:34:19Z

Keeping this change in a separate commit makes sense to me.

You're right, the XVATTR code really should go away. None of it makes sense for Linux. What's kept anyone from doing this so far is that the needed changes go much deeper than you might initially expect. And as you rightly guessed we've left it largely for reasons of correctness. That said, I wouldn't be at all opposed to seeing it go and that might be more practical now that we have better test coverage.

lorddoskias · 2016-08-16T21:43:04Z

@behlendorf I see, indeed it's going to be tricky. But looking even further I'm really perplexed as to how the mtime stuff should be handled. Since first we have:

if (mask & ATTR_MTIME) {
        ZFS_TIME_ENCODE(&vap->va_mtime, mtime);
        SA_ADD_BULK_ATTR(bulk, count, SA_ZPL_MTIME(zsb), NULL,
            mtime, sizeof (mtime));
    }

Meaning whatever is the value passed from the VFS is going to be persisted on disk. Then in the else branch, by using zfs_tstamp_update_setup actually a different time - whatever "current" time is at the invocation of zfs_tstamp_update. Maybe this is a moot point, but we essentially make i_mtime and whatever is persisted on disk unsynchronized. I wonder whether that might be problematic in certain cases.

behlendorf · 2016-08-16T22:16:40Z

I wonder whether that might be problematic in certain cases.

That's troubling. That certainly could cause subtle issues. For instance things like tar and make really don't like it when things go back in time. Although that's not likely to happen unless we're very unlucky about when the inode gets dropped from the cache.

To my knowledge I don't recall any reports of this causing problems but it's wouldn't be a terrible idea to address it.

lorddoskias · 2016-08-22T20:57:25Z

@behlendorf Actually I might be wrong. I just tested commenting the code in the "else" branch and always relying on whatever is passed into vap->va_mtime and the test kept failing. So what's in here now seems to be correct. I will squash the commits, rebase and resubmit and if everything is ok I guess this can be merged.

behlendorf · 2016-08-22T21:13:50Z

Yup, just make sure you get the cstyle issue too.

lorddoskias · 2016-08-23T10:05:51Z

So the non-simd test seemed to just have timeout. However, I cannot understand the leakage in the SIMD instance. I cannot see how my changes could affect this. Is this just some instance jitter?

behlendorf · 2016-08-23T17:57:10Z

The non-SIMD failure was due to #4034 which is a rare longstanding issue we've seen in testing a few times. The leaking in the SIMD case I think is unrelated too. I've resubmitted both of those builds to see if it's reproducible.

tuxoko · 2016-08-24T18:23:09Z

the time update in setattr is over complicated. It should be as simple as setattr_copy.
http://lxr.free-electrons.com/source/fs/attr.c#L142

behlendorf · 2016-08-24T21:18:23Z

There's definitely room for simplifying this. But I think it makes sense to make that kind of change in the context of switching from the illumos vattr to the linux iattr as mentioned above.

This change as is LGTM. @tuxoko do you have any remaining concerns about this cleanup?

tuxoko · 2016-08-24T21:29:57Z

@behlendorf
As a matter of fact I do. It just doesn't make sense that we use zfs_tstamp_update_setup in setattr. We either use the value passed in iattr or we don't touch it. And the if (mask != 0) seems like we will wipe the mtime to current time no matter what.

lorddoskias · 2016-08-25T16:52:36Z

@tuxoko I tested an implementation that was very similar to what copy_attr was doing w.r.t to time handling and it still was failing the time test from ZFS. Though I agree that logic should be fairly simple. I will have another go at that, hopefully bringing better results. And you are right about your concern of ignoring what's in the iattr when mask != 0 and just using the current time.

tuxoko · 2016-08-29T18:24:37Z

@lorddoskias
I think you didn't update mtime, ctime in inode before, that's why tests failed. The atime is done in zpl_setattr.

lorddoskias · 2016-08-30T18:29:07Z

@tuxoko You were right. I just did the code, but this time with the proper inode modification and the test passed. I've pushed a new branch, hopefully this one is the final version.

behlendorf · 2016-08-30T18:51:22Z

module/zfs/zfs_vnops.c

@@ -3010,29 +3009,27 @@ zfs_setattr(struct inode *ip, vattr_t *vap, int flags, cred_t *cr)

 	if (mask & ATTR_MTIME) {
 		ZFS_TIME_ENCODE(&vap->va_mtime, mtime);
+		ZTOI(zp)->i_mtime = timespec_trunc(vap->va_mtime,
+				ZTOI(zp)->i_sb->s_time_gran);


[cstyle] 4 space indent for continued lines. The same change should be made to the rest of the patch.

Strance, my cstyle doesn't report anything, that's why I pushed it. Are you using a modified cstyle or something?

No, I just noticed while looking at the patch. cstyle just isn't quite smart enough to catch this case. Unlike the Linux kernel style the SunOS style is to simply indent 4 spaces to continue a line.

Ok, goot thing vim's "set list" option shows the discrepancies. Fixed in current version but will push once tests are finished so that I get a baseline results

tuxoko · 2016-08-30T19:18:28Z

I noticed that zfs_log_setattr doesn't store ctime, and it doesn't bother to do anything with ATTR_CTIME, heck, the original zfs_setattr doesn't bother either. So I guess in order for log replay to work, we need to revert the ctime part.

lorddoskias · 2016-08-30T19:31:45Z

@tuxoko You mean just remove the if (mask & ATTR_CTIME) branch? Once the current batch of tests finish I will do that and test locally and then resubmit. I want to at get some baseline results with the current version of the code.

tuxoko · 2016-08-30T19:44:43Z

@lorddoskias
I mean revert the ctime to if (mask !=0) and use zfs_tstamp_... to get current time, since we don't have the target ctime when doing replay.

behlendorf · 2016-08-30T20:01:31Z

I noticed that zfs_log_setattr doesn't store ctime

That's really surprising, and the replay code here is downright convoluted. It would be so much clearer if zfs_replay_setattr just set vap->va_ctime to now and set ATTR_CTIME. Then we wouldn't need this crazy special case handling in zfs_setattr() for this replay case.

lorddoskias · 2016-08-30T20:16:35Z

@behlendorf I thought the same thing as I was reading the code. Any particular reason why this isn't the case? It seems a rather simple fix for the case? Any objections to doing it that way in zfsonlinux?

tuxoko · 2016-08-30T20:25:49Z

Yeah, this would be much cleaner.

behlendorf · 2016-08-30T20:32:38Z

I've no idea was it was originally done this way, this logic predates my involvement with ZFS. I don't have any objection to updating the code for ZoL to do the simpler thing here.

tuxoko · 2016-08-30T21:32:10Z

BTW, I'm kind of curious about the mask in zfs_log_setattr. Is this even OS independent. It sure doesn't look like.

behlendorf · 2016-08-30T21:39:20Z

No, it's definitely not designed to be OS independent. It's just adapted from the original Solaris code.

tuxoko · 2016-08-30T21:40:49Z

Which means a pool with dirty log is not safe to migrate between OS?

behlendorf · 2016-08-30T21:47:23Z

No that should behave consistently across OSs. We interpret the log contents the same way and map them to their Linux counterparts. But as you know a lot of this infrastructure is Solaris specific and is just emulated on Linux (vattr_t, xvattr_t, etc). It's be great to just do the native Linux thing or alternately some more OS independent interface we could all use.

tuxoko · 2016-08-30T21:55:46Z

I'm talking about the mask value. I believe we just use the ATTR_UID and friends defined in Linux without any translation. This surely won't behave correctly when imported in Solaris.

behlendorf · 2016-08-30T22:01:52Z

Oh, I see. You're right, those definitely don't match up and really should be translated.

ZFS doesn't provide a custom update_time method meaning it delegates this job to the generic VFS layer. The only time when it needs to set the various *time values is when the inode is being marshalled to/from the disk. Do this by moving the relevant code from zfs_inode_update_impl to zfs_node_alloc and zfs_rezget. As a result from this change it is no longer necessary to have multiple versions of the zfs_inode_update function - so just nuke them and leave only one.

lorddoskias · 2016-09-12T14:35:02Z

The failures seem irrelevant to the changes.

tuxoko · 2016-09-12T17:21:56Z

include/sys/zil.h

@@ -320,6 +320,7 @@ typedef struct {
 	uint64_t	lr_size;	/* size to set */
 	uint64_t	lr_atime[2];	/* access time */
 	uint64_t	lr_mtime[2];	/* modification time */
+	uint64_t	lr_ctime[2];	/* change time */


Don't change the log record structure.

tuxoko · 2016-09-12T17:26:15Z

That's really surprising, and the replay code here is downright convoluted. It would be so much clearer if zfs_replay_setattr just set vap->va_ctime to now and set ATTR_CTIME. Then we wouldn't need this crazy special case handling in zfs_setattr() for this replay case.

@lorddoskias Please try this way.

Simplify time handling in zfs_setattr by mimicking the logic in setattr_copy from the linux kernel. In order to achieve this in the case when ZFS' log is being replayed it is necessary to unconditionally set the ctime in zfs_replay_setattr. Also use the timespec_trunc function when assigning values to the generic inode struct. This is currently a noop since zfs sets s_time_gran to 1, however in the future rules about precision might change.

lorddoskias · 2016-09-12T19:39:01Z

@tuxoko Should be looking much better now. What do you say ?

tuxoko · 2016-09-13T00:42:37Z

@lorddoskias
LGTM, thanks.

behlendorf · 2016-09-13T18:56:25Z

That's so much better, LGTM.

Simplify time handling in zfs_setattr by mimicking the logic in setattr_copy from the linux kernel. In order to achieve this in the case when ZFS' log is being replayed it is necessary to unconditionally set the ctime in zfs_replay_setattr. Also use the timespec_trunc function when assigning values to the generic inode struct. This is currently a noop since zfs sets s_time_gran to 1, however in the future rules about precision might change. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Chunwei Chen <[email protected]> Signed-off-by: Nikolay Borisov <[email protected]> Closes #4916

behlendorf · 2016-09-13T19:16:45Z

Merged as:

87f9371 Simplify time handling logic in zfs_settattr
9f5f001 Refactor generic inode time updating

Only i_mode, i_blocks, and i_size_write() to go.

lorddoskias force-pushed the time-handling-cleanup branch from fc34576 to 6b1e4bc Compare August 9, 2016 18:58

lorddoskias reviewed Aug 9, 2016
View reviewed changes

behlendorf reviewed Aug 11, 2016
View reviewed changes

lorddoskias force-pushed the time-handling-cleanup branch 2 times, most recently from d3ca60e to 8f12781 Compare August 15, 2016 18:49

lorddoskias force-pushed the time-handling-cleanup branch from 8f12781 to 07a04ed Compare August 22, 2016 21:02

behlendorf added this to the 0.7.0 milestone Aug 22, 2016

lorddoskias force-pushed the time-handling-cleanup branch from 07a04ed to a990133 Compare August 22, 2016 21:22

lorddoskias force-pushed the time-handling-cleanup branch from b219bf0 to 441cc8f Compare August 30, 2016 18:36

behlendorf reviewed Aug 30, 2016
View reviewed changes

lorddoskias force-pushed the time-handling-cleanup branch from 441cc8f to bd5a37b Compare September 10, 2016 18:07

tuxoko reviewed Sep 12, 2016
View reviewed changes

lorddoskias force-pushed the time-handling-cleanup branch from bd5a37b to e561a26 Compare September 12, 2016 19:36

behlendorf closed this in 9f5f001 Sep 13, 2016

Refactor generic inode time updating #4916

Refactor generic inode time updating #4916

Conversation

lorddoskias commented Aug 1, 2016

tuxoko commented Aug 1, 2016 • edited Loading

behlendorf commented Aug 2, 2016

lorddoskias commented Aug 9, 2016

Choose a reason for hiding this comment

tuxoko Aug 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorddoskias commented Aug 11, 2016

behlendorf commented Aug 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

behlendorf commented Aug 15, 2016

lorddoskias commented Aug 16, 2016 • edited Loading

behlendorf commented Aug 16, 2016 • edited Loading

lorddoskias commented Aug 16, 2016

behlendorf commented Aug 16, 2016

lorddoskias commented Aug 16, 2016

behlendorf commented Aug 16, 2016

lorddoskias commented Aug 22, 2016

behlendorf commented Aug 22, 2016

lorddoskias commented Aug 23, 2016

behlendorf commented Aug 23, 2016

tuxoko commented Aug 24, 2016

behlendorf commented Aug 24, 2016

tuxoko commented Aug 24, 2016

lorddoskias commented Aug 25, 2016

tuxoko commented Aug 29, 2016 • edited Loading

lorddoskias commented Aug 30, 2016

behlendorf Aug 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuxoko commented Aug 30, 2016

lorddoskias commented Aug 30, 2016

tuxoko commented Aug 30, 2016

behlendorf commented Aug 30, 2016

lorddoskias commented Aug 30, 2016

tuxoko commented Aug 30, 2016

behlendorf commented Aug 30, 2016

tuxoko commented Aug 30, 2016 • edited Loading

behlendorf commented Aug 30, 2016

tuxoko commented Aug 30, 2016

behlendorf commented Aug 30, 2016

tuxoko commented Aug 30, 2016

behlendorf commented Aug 30, 2016

lorddoskias commented Sep 12, 2016

Choose a reason for hiding this comment

tuxoko commented Sep 12, 2016

lorddoskias commented Sep 12, 2016

tuxoko commented Sep 13, 2016

behlendorf commented Sep 13, 2016

behlendorf commented Sep 13, 2016 • edited Loading

tuxoko commented Aug 1, 2016 •

edited

Loading

tuxoko Aug 9, 2016 •

edited

Loading

behlendorf commented Aug 11, 2016 •

edited

Loading

lorddoskias commented Aug 16, 2016 •

edited

Loading

behlendorf commented Aug 16, 2016 •

edited

Loading

tuxoko commented Aug 29, 2016 •

edited

Loading

behlendorf Aug 30, 2016 •

edited

Loading

tuxoko commented Aug 30, 2016 •

edited

Loading

behlendorf commented Sep 13, 2016 •

edited

Loading