[00:04:59] *** Quits: Param (~Param@106.51.65.217) (Quit: Param) [00:42:02] *** Joins: Param (~Param@106.51.65.217) [01:31:30] *** Joins: gila (~gila@5ED4D9C8.cm-7-5d.dynamic.ziggo.nl) [02:32:29] *** Quits: Shuhei (caf6fc61@gateway/web/freenode/ip.202.246.252.97) (Ping timeout: 260 seconds) [05:22:35] *** Quits: Param (~Param@106.51.65.217) (Quit: Param) [06:40:52] *** Joins: Param (~Param@157.49.5.23) [06:46:01] *** Quits: Param (~Param@157.49.5.23) (Quit: Param) [06:46:46] *** Joins: Param (~Param@157.49.0.87) [06:58:32] *** Quits: Param (~Param@157.49.0.87) (Quit: Param) [07:29:09] *** Joins: Param (~Param@157.49.0.253) [07:54:22] drv: about https://review.gerrithub.io/#/c/386114/, can we use pthread_getspecific/pthread_setspecific for per thread variables? [08:00:04] *** Joins: lhodev (~Adium@66-90-218-190.dyn.grandenetworks.net) [08:01:16] *** Parts: lhodev (~Adium@66-90-218-190.dyn.grandenetworks.net) () [09:08:19] pwodkowx: hmm, that might be doable, although it would be nice to keep all of the pthread calls isolated to io_channel.c if possible [09:08:44] maybe we can register an io_device for errno translation and use io_channel to allocate the per-thread buffers [09:09:08] although we probably don't want to tie strerror translation to having a spdk_thread available [09:12:22] maybe we should just move the strerror implementation to its own file and use __thread so that it is easy to replace for people who don't have it... [09:13:03] ^^ this [09:13:23] I think that idea plus pwodkowx's patch [09:13:35] is the best approach [09:16:23] sounds good to me [09:38:16] *** Quits: Param (~Param@157.49.0.253) (Quit: Param) [10:46:58] *** Parts: darsto__ (~dstojacx@89-68-135-211.dynamic.chello.pl) ("Leaving") [10:47:07] *** Joins: darsto__ (~dstojacx@89-68-135-211.dynamic.chello.pl) [11:00:38] bwalker, jimharris: I took your +2s off of https://review.gerrithub.io/#/c/389855/ - but I think if we merge that plus the next patch, my concerns will be taken care of [11:00:43] please re-review [11:04:30] lgtm [11:11:46] drv: can you clean up this nit from mszwed? https://review.gerrithub.io/#/c/389894/ [11:13:30] ok, uploaded a new rev [11:15:00] *** Joins: lhodev (~Adium@66-90-218-190.dyn.grandenetworks.net) [11:15:55] *** Parts: lhodev (~Adium@66-90-218-190.dyn.grandenetworks.net) () [11:27:15] *** Joins: lhodev (~Adium@inet-hqmc01-o.oracle.com) [11:27:30] *** Parts: lhodev (~Adium@inet-hqmc01-o.oracle.com) () [11:56:30] *** Joins: lhodev (~Adium@inet-hqmc04-o.oracle.com) [11:57:03] *** Parts: lhodev (~Adium@inet-hqmc04-o.oracle.com) () [12:55:11] bwalker, drv: gerrit doesn't seem to have a way to search for patches with no assignee [12:55:25] things like assignee:"" return errors [12:55:44] guess you have to write a dashboard that makes its own REST queries [12:55:53] ha [12:57:19] this one removes patches with a -1 from the main review list though: [12:57:22] https://review.gerrithub.io/#/dashboard/?Outgoing=projects:spdk+o:self+status:open&Needs%20My%20%2B2=projects:spdk+-o:self+status:open+label:Verified%253D1+label:Code-Review%253D2+-label:Code-Review%253D2%252Cself&Waiting%20For%20Another%20%2B2=projects:spdk+-o:self+status:open+label:Verified%253D1+label:Code-Review%253D2+label:Code-Review%253D2%252Cself&Needs%20Review=projects:spdk+-o:self+status:open+label:Verified%253E%253D1+-label:Code-Rev [12:57:22] iew%253E%253D2+-label:Code-Review%253D-1&Open=projects:spdk+status:open+-o:self+(-label:Verified%253E%253D1+OR+label:Code-Review%253D-1)&Merged=projects:spdk+status:merged [13:01:13] while you're at it, can you make a section for projects that start with spdk but aren't the main spdk repo? [13:02:11] sure - how about I just group all open patches into that section? [13:02:22] meaning don't differentiate based on Verified, Code Review, etc. [13:02:50] but I want to filter out ones with a -1 of any kind, but not ones that aren't verified yet because they don't have a CI system [13:03:14] basically, any repo that isn't the main SPDK repo I want to show up together because there aren't many and I should review those immediately [13:12:30] https://review.gerrithub.io/#/dashboard/?Outgoing=projects:spdk+o:self+status:open&Needs%20My%20%2B2=projects:spdk+-o:self+status:open+label:Verified%253D1+label:Code-Review%253D2+-label:Code-Review%253D2%252Cself&Waiting%20For%20Another%20%2B2=projects:spdk+-o:self+status:open+label:Verified%253D1+label:Code-Review%253D2+label:Code-Review%253D2%252Cself&Needs%20Review=projects:spdk+-o:self+status:open+(label:Verified%253E%253D1+OR+-project:sp [13:12:30] dk/spdk)+-label:Code-Review%253E%253D2+-label:Code-Review%253D-1&Open=projects:spdk+status:open+-o:self+((-label:Verified%253E%253D1+AND+project:spdk/spdk)+OR+label:Code-Review%253D-1)&Merged=projects:spdk+status:merged [13:17:08] yeah that's going to help a lot [13:17:16] I dropped the "merged" section [13:18:20] yeah, i may drop that too - i do use it sometimes but when i do it's easy enough to just type out that search [13:20:31] can we drop every patch out for the spdk/tests repo? [13:22:42] i was going to mark them -1 [13:23:03] i guess i could add more clauses though to the url [13:23:46] actually that's pretty easy - I only need to add it to the Needs Review section [13:24:16] I didn't mean you [13:24:19] I meant abandon [13:24:34] we've change direction to where the tests are going into spdk/spdk instead [13:24:45] I guess I could just merge them all [13:24:48] if they don't conflict [13:25:06] https://review.gerrithub.io/#/dashboard/?Outgoing=projects:spdk+o:self+status:open&Needs%20My%20%2B2=projects:spdk+-o:self+status:open+label:Verified%253D1+label:Code-Review%253D2+-label:Code-Review%253D2%252Cself&Waiting%20For%20Another%20%2B2=projects:spdk+-o:self+status:open+label:Verified%253D1+label:Code-Review%253D2+label:Code-Review%253D2%252Cself&Needs%20Review=projects:spdk+-project:spdk/tests+-o:self+status:open+(label:Verified%253E% [13:25:06] 253D1+OR+-project:spdk/spdk)+-label:Code-Review%253E%253D2+-label:Code-Review%253D-1&Open=projects:spdk+status:open+-o:self+((-label:Verified%253E%253D1+AND+project:spdk/spdk)+OR+label:Code-Review%253D-1)&Merged=projects:spdk+status:merged [13:38:08] oops - I was wrong, there is "is:assigned" [13:38:19] how did I miss that? [13:48:51] *** Joins: lhodev (~Adium@inet-hqmc08-o.oracle.com) [13:48:57] *** Parts: lhodev (~Adium@inet-hqmc08-o.oracle.com) () [14:03:02] try this one: [14:03:03] https://review.gerrithub.io/#/dashboard/?Outgoing=projects:spdk+o:self+status:open&Needs%20My%20%2B2=projects:spdk+-o:self+status:open+label:Verified%253D1+label:Code-Review%253D2+-label:Code-Review%253D2%252Cself&Waiting%20For%20Another%20%2B2=projects:spdk+-o:self+status:open+label:Verified%253D1+label:Code-Review%253D2+label:Code-Review%253D2%252Cself&Needs%20Review=projects:spdk+-project:spdk/tests+-o:self+status:open+(label:Verified%253E% [14:03:03] 253D1+OR+-project:spdk/spdk)+(assignee:self+OR+-is:assigned+OR+label:Code-Review%253D1)+-label:Code-Review%253E%253D2+-label:Code-Review%253D-1&Open=projects:spdk+status:open+-o:self+label:Verified%253D1+-label:Code-Review%253D2 [14:03:17] for "Open", I just removed any that hadn't passed the test pool [14:10:09] so now reviews that are assigned but not +1 don't show up anywhere, right? [14:10:14] assigned to someone else, that is [14:10:46] they'll show up under Open [14:11:03] but not "Needs Review" [14:11:15] that will work [14:12:46] so some of the "Needs Review" patches show up under Open [14:12:50] duplicated [14:13:06] that's not the end of the world - Open just means everything Open that passed the tests [14:13:39] and there's no where to see reviews that failed the tests for spdk/spdk, but that's probably for the best [14:14:11] i never look for those in the dashboard [14:14:28] i may accidentally look at such a patch as part of seeing it in the chain of some other patch [14:14:39] yeah - for my personal use I may just drop the "Open" section from your query [14:14:53] I only used it before to scan for patches that weren't in spdk/spdk [14:14:58] yes - they'll be duplicated - my boolean logic synapses were misfiring [14:17:38] bwalker: can you look at https://review.gerrithub.io/#/c/388293/11/lib/nvmf/nvmf.c? [14:20:17] I just responded to you [14:22:28] i'm still not following [14:22:46] if the user adds two namespaces synchronously - shouldn't the second one fail in this function? [14:23:07] if they have the same ns->id? [14:23:34] sorry - the two is a red herring. That's just what the test does. The race is between creating the subsystem and adding a namespace [14:23:52] the API to create a new subsystem and to add a namespace both appear to be synchronous [14:24:02] but they're not in reality - they send events to each poll group [14:24:13] (I'll change the API, but it forces me to rewrite the config file parsing in nvmf_tgt) [14:24:32] so if the target does create subsystem, then immediately add namespace [14:24:46] the event from the create subsystem that executes on the poll group will see that the subsystem has a namespace [14:24:50] and add it to the poll group [14:24:52] oh - so you mean poll group 1 would set channels[ns_idx] to something [14:25:05] yes - because it is present in the subsystem when the subsystem is added [14:25:19] because the user didn't actually wait for adding the subsystem to complete [14:25:23] wait - I'm still confused - the other poll group has its own array of channels, right? [14:25:24] because the API returns immediatley [14:25:32] this can all happen on one thread with one poll group [14:25:46] the order of events is 1) user calls add subsystem, which generates an event to itself and returns [14:26:10] 2) user calls add subsystem, which updates the subsystem data structure with a new namespace, and generates an event to tell the poll group and returns [14:26:30] 3) event from #1 executes, and adds the subsystem to the poll group. It already sees the new namespace, so it creates channels for it [14:26:32] 2) user calls add subsystem again? [14:26:39] or do you mean 2) user calls add namespace? [14:26:48] 2) add namespace - sorry [14:27:01] 4) event from #2 executes, tries to add the namespace again [14:27:29] so then my first thought to fix it was make it so only calling add namespace actually creates I/O channels, not add subsystem [14:27:30] why does the event from #1 have to create the channel? [14:27:43] but if you want to dynamically create poll groups too [14:27:48] later on [14:27:54] when the poll group is created, it adds each subsystem [14:28:00] and has to create an I/O channel for each namespace [14:28:15] I could make that code work by sending an event for each subsystem, which sends an event for each namespace [14:28:24] or I could make the add subsystem code just do it inline [14:28:28] it's really a code sharing thing [14:28:58] ok - I follow now [14:29:14] can you add some comments in that function though [14:29:16] it's a mess - if it were asynchronous and only returned the handle after it was really done [14:29:18] then it would be fine [14:29:29] because you couldn't add the namespace until the events propagated for the subsystem [14:29:41] so that's my long term direction [14:29:51] but then I have to rewrite the config file parser to be asynchronous [14:30:22] these nvmf changes have just been constant chicken or egg problems [14:34:42] https://review.gerrithub.io/#/c/389641/2/app/nvmf_tgt/nvmf_tgt.c [14:35:10] so the idea here is that we don't have to wait for all of the subsystems to get deleted - we can immediately move to tgt_destroy? [14:36:48] the poll groups have all been destroyed by that point [14:36:52] there is nothing to wait for [14:37:26] it's just freeing memory at that point now [14:37:48] why did it wait until they were all deleted before this patch? [14:38:16] deleting the subsystem used to have to put I/O channels [14:38:21] which had to be done from the thread they were obtained on [14:38:24] so you had to send messages [14:38:40] but now we are using the I/O channels from the poll groups [14:38:41] ah - the i/o channel stuff is now all done via the poll groups, not the subsystems? [14:38:46] as of that patch, yes [14:38:57] so the poll group shutdown sequence is all asynchronous [14:39:10] but the subsystem is just global data that can be released synchronously now [14:39:48] sounds good - i like any patch with a line count like this: +6, -167 [14:42:27] I pushed the first 3 patches in your nvmf series - so you'll probably want to rebase from master before pushing a new set with that memset zero fix [15:19:37] I think I just fixed the bug blocking the last patch in that series [15:19:44] which officially moves everything to the new threading model [15:20:01] it was already passing the tests for the plain nvmf stuff, but was failing with the combo iscsi+nvmf tests [15:20:09] so now that I fixed that, I'm reasonably confident we're there [16:05:47] *** Quits: guerby (~guerby@april/board/guerby) (Remote host closed the connection) [16:08:37] *** Joins: guerby (~guerby@april/board/guerby) [16:08:56] *** Quits: guerby (~guerby@april/board/guerby) (Remote host closed the connection) [16:09:08] *** Joins: guerby (~guerby@april/board/guerby) [20:10:11] *** Joins: Param (~Param@223.227.46.196) [20:23:08] *** Quits: Param (~Param@223.227.46.196) (Quit: Param) [20:37:30] *** Joins: Param (~Param@223.227.46.196) [20:54:05] *** Quits: Param (~Param@223.227.46.196) (Quit: Param) [21:21:02] *** Joins: Param (~Param@223.227.46.196) [21:22:07] *** Quits: Param (~Param@223.227.46.196) (Client Quit) [21:28:17] *** Joins: Param (~Param@223.227.46.196) [22:13:36] *** Quits: Param (~Param@223.227.46.196) (Quit: Param)