[00:15:15] *** Joins: tomzawadzki (~tzawadzk@192.55.54.44) [01:38:21] *** Joins: gila (~gila@5ED4FE92.cm-7-5d.dynamic.ziggo.nl) [01:56:44] *** Quits: tsuyoshi (b42b2067@gateway/web/freenode/ip.180.43.32.103) (Quit: Page closed) [02:27:30] *** Quits: gila (~gila@5ED4FE92.cm-7-5d.dynamic.ziggo.nl) (*.net *.split) [02:29:55] *** Quits: changpe1 (~changpe1@192.55.54.44) (Ping timeout: 240 seconds) [02:30:36] *** Joins: changpe1 (~changpe1@192.55.54.44) [02:54:14] *** Joins: gila (~gila@5ED4FE92.cm-7-5d.dynamic.ziggo.nl) [03:02:26] *** Quits: gila (~gila@5ED4FE92.cm-7-5d.dynamic.ziggo.nl) (Quit: My Mac Pro has gone to sleep. ZZZzzz…) [06:03:53] *** Joins: gila (~gila@5ED4FE92.cm-7-5d.dynamic.ziggo.nl) [07:04:43] *** ChanServ sets mode: +o peluse [08:40:11] *** Joins: johnmeneghini (~johnmeneg@pool-96-252-112-122.bstnma.fios.verizon.net) [08:53:44] *** Quits: gila (~gila@5ED4FE92.cm-7-5d.dynamic.ziggo.nl) (Quit: My Mac Pro has gone to sleep. ZZZzzz…) [08:57:19] what is our policy on updating the fio rev for our plugins? [08:57:44] i created this blobfs fio plugin and was using fio-2.19 without realizing our nvme plugin is based on 2.18 [08:58:32] there is an engine_data field in fio_file that was switched from uint64_t to void * - i'm going to just change my plugin to explicitly cast (which should work for both) [08:58:54] jimharris, have you ever tried building with CONFIG_COVERAGE=y in FreeBSD? [08:59:05] nope [08:59:40] freebsd is pretty low priority - so i'm sure there are some things that may not work on freebsd [08:59:49] OK, doesn't like it for some reason so will dork with it for a little while then punt for now [09:00:03] we haven't had a lot (or any?) requests for it [09:00:06] I've just rebased and updated review https://review.gerrithub.io/#/c/362604/ [09:00:40] What does the following message mean, and how can I fix it? Change https://review.gerrithub.io/362604 - Needs Verified Label [09:01:10] How do I add a label to my review request? [09:01:34] you don't add this label - it gets added by the CI system once it tests your patch [09:01:56] OK. So I do nothing. :-) [09:02:04] exactly :) [09:40:13] After updating my patch I see that I am still conflicting with 7 other changes. [09:40:50] that's fine - it just means that if we get your patch in first, those 7 patches will have merge conflicts [09:40:53] or vice versa [09:41:28] i'm making a few edits to your patch (fixing some of the line breaks in blobstore.c, and changing the env.c changes to renames only - reverting the code changes you made) [09:41:41] then we will get this patch in today [09:42:32] I didn't see the line breaks in blobstore. Is this something check_format.sh was supposed to catch? [09:43:17] not really - you had to break the lines in a few places to keep them below 100 characters but the breaks didn't end up in a natural place [09:43:29] i'm just adding a mask_size variable to make the code look cleaner [09:44:05] the env.c changes though would have been better to just do a renaming, instead of changing the implementation of the functions [09:48:06] patch looks great johnmeneghini - i just pushed a few changes to it [09:50:04] bwalker, drv: can you take a look at johnmeneghini's patch? https://review.gerrithub.io/#/c/362604/ [09:51:04] OK. So you're saying I should have (e.g.) renamed spdk_malloc_socket to spdk_dma_malloc_socket rather changing the content of the whole thing to spdk_dma_malloc() and then adding the function again later. Sorry about that. This was kind of messy merge conflict in this file. [09:57:28] no worries john - really appreciate you taking on this very big but very pedantic patch [10:13:47] *** Joins: sethhowe (sethhowe@nat/intel/x-gdujudewevgmnkwk) [10:30:50] *** Quits: sethhowe (sethhowe@nat/intel/x-gdujudewevgmnkwk) (Remote host closed the connection) [10:32:48] *** Joins: sethhowe (~sethhowe@192.55.54.38) [10:42:22] I am in a training class this morning, but I'll take a look this afternoon [10:53:43] ok - is bwalker around? [11:27:55] I think bwalker is on a customer visit today [12:24:44] yeah he is [12:25:31] * peluse thought drv was pretty well trained already... [12:33:10] *** Quits: tomzawadzki (~tzawadzk@192.55.54.44) (Ping timeout: 268 seconds) [12:35:07] in in gerrit when I'm replying to a comment I seem to have to hit the back button on my browser to post my replies, is that the same for everyone else? [12:36:40] you mean instead of hitting the up arrow in the upper right to go back to the main review? [12:39:09] hmm, maybe I'm just not seeing the up arrow... will go look [12:39:36] well i see that problem - if i scroll down on the main review page, then click on a file... [12:39:56] ...from the file view, i cannot scroll up to get the arrows - and have to either refresh or hit the back button [12:41:13] I had to zoom out to see the arrow, then when I zoomed back in it was still visible. strange... oh back, the back button works as well [12:51:25] the keyboard shortcuts are super useful too - hit '?' and it will show you the whole list [12:51:53] mostly vim-style navigation, and you can hit 'u' to go up to the review from a diff or 'a' to directly open up the Reply... box [12:54:58] *** Joins: gila (~gila@5ED4FE92.cm-7-5d.dynamic.ziggo.nl) [13:00:48] drv, nice, thanks [14:19:23] *** Quits: gila (~gila@5ED4FE92.cm-7-5d.dynamic.ziggo.nl) (Quit: My Mac Pro has gone to sleep. ZZZzzz…) [14:22:13] I'm circling back to some of the UT stuff I started a while back and will being with a small patch introducing the --wrap ability but in a new file under spdk/test/spdk_mock.c and after getting just the wraps in there we can use it to expand mocking where wrap isn't used. Any input of name and/or location of new file? [14:23:15] i'm just happy that we have you pushing patches directly to gerrithub now - that's a big step compared to where the project was just a month ago [14:23:21] ugh [14:23:23] wrong window [14:24:55] could have been worse :) [14:24:59] much worse [14:25:03] LOL [14:25:58] i think first you want some new .mk makefile under the mk directory that can be included to get all of the --wrap definitions [14:26:10] follow-up to my question on the new test .c file, preferred location for the matching .h? I see an include/spdk and an include/spdk_internal but not obvious to me based on what's in there what the criteria is for placement [14:26:10] so we don't duplicate those in all of the individual unit test makefiles [14:26:22] well, yeah that will be part of it [14:26:51] include/spdk is for headers that are part of the public API - i.e. applications that are including SPDK [14:26:59] so this patch will be the .c, the .h, the .mk needed for any test to use --wrap [14:27:00] include/spdk_internal is more for inter-library dependencies [14:27:17] OK, so that's a good palce for this test_mock.h file? [14:27:23] place palce, blah [14:27:48] yeah - i think that's as good a place as any for the header file [14:27:58] K [14:28:17] we have the cunit test code under lib/cunit - so maybe a new lib/ut_mock directory? [14:28:37] this gets back to your point about reorganizing the directory structure a bit [14:28:50] i think really this cunit and mock code belongs somewhere under test [14:30:21] yeah, that's better than just dropping it under test where there's just one other C file [14:31:14] and then if drv wants to make it a subdir of some other UT thing somewhere it'll be easy to shuffle around [14:31:47] i'll bet that's what he's getting trained for today - how to organize directory hierarchies effectively [14:32:00] i wonder if there is a certification for that? [14:32:58] if not we should make one [14:33:50] i got nothin' :) [14:52:45] the training was about technical career pipeline stuff, but I would sign up for that directory organization one :) [15:24:41] johnmeneghini, jimharris: bwalker and I made some comments on the dma_malloc review [15:24:56] we should come to an agreement about the naming [15:25:18] I think the simplest thing is to just get rid of the zmalloc variants and make malloc() always return zeroed memory [15:25:31] we almost always use zmalloc already [15:34:34] So you want every call to spdk_malloc to always zero the memory? [15:34:56] well dpdk does that under the covers already today [15:35:12] but i'm leaning towards keeping both variants [15:40:59] alright, I'm OK with that too, but like bwalker said, now is our chance to rename stuff since we are breaking API anyway [15:43:25] ok - johnmeneghini - are you ok with spdk_dma_alloc and spdk_dma_zalloc? we can make these changes to your patch tomorrow [15:55:25] I would actually lean toward spdk_dma_malloc rather than spdk_dma_alloc, but I don't have a very strong preference [15:59:21] i could go either way - dma_malloc means we can push john's patch now :) [15:59:24] I'm good with calling these what ever you want. But I do think we should keep a separate zero'd and non-zero'd API. [16:00:51] Jim, since you pushed the last patch - patch 3 - maybe you can make this final change and push in patch 4. [16:01:24] if we can get bwalker to agree on spdk_dma_malloc (as you originally renamed it), then patch #3 should be the final candidate [18:35:57] jimharris: have you seen this RocksDB abort before? http://spdk.intel.com/public/spdk/builds/review/2053e0639c289c85a5fd5420aa8351b163c22504.1496194375/wkb-fedora-04/build.log [18:36:22] I don't think it is triggered by the patch - it only changes vhost stuff: https://review.gerrithub.io/#/c/362505/ [18:37:31] oh - this is that primary/secondary issue i was talking about earlier - i've already asked darek and pawel to rebase that series on top of the changes i made where we wait to start secondary processes until the stub app creates a sentinel file indicating it has finished spdk and nvme init [18:37:48] oh, OK [20:30:07] jimharris: I think there is a deadlock somewhere in blobfs_sync_ut - it has been hitting on some of the Travis runs, and I just reproduced it now [20:31:57] https://gist.github.com/dverkamp-intel/3290e01bc5f6706403aee562ee63756c [20:33:01] it's intermittent i assume? [20:33:41] yeah [20:34:08] I assume there is some kind of lock ordering issue - the unit test has a mutex, and the library has a lock too, so if we take them in opposite orders somewhere, the threads can deadlock [20:34:30] I guess the lock in the test was added fairly recently by bwalker [20:36:13] the pthread_spin_unlock at blobfs.c:1865 - can you try moving that before the call to __rw_send_from_file? [20:38:46] that looks good at first glance [20:39:20] the hang reproduced after a few seconds of running `while true; do test/lib/blobfs/blobfs_sync_ut/blobfs_sync_ut; done`, and hasn't reproduced with the change [20:39:46] i'll need to scrub other calls to send_request [20:40:03] i'll push a patch for that fix here shortly [20:40:07] I didn't look if that is actually correct in terms of thread safety, but the unit tests all pass :) [20:42:24] that fix is fine for thread safety - there is no reason that call has to be done under the lock [21:35:03] bwalker: ping [21:43:44] *** Joins: gila (~gila@5ED4FE92.cm-7-5d.dynamic.ziggo.nl) [22:03:08] *** Quits: gila (~gila@5ED4FE92.cm-7-5d.dynamic.ziggo.nl) (Quit: My Mac Pro has gone to sleep. ZZZzzz…) [22:06:24] *** Joins: ziyeyang_ (~ziyeyang@134.134.139.83)