[00:53:37] *** Joins: gila (~gila@5ED4FE92.cm-7-5d.dynamic.ziggo.nl) [01:48:05] *** Quits: ziyeyang__ (~ziyeyang@192.55.54.42) (Ping timeout: 240 seconds) [01:49:23] *** Joins: ziyeyang_ (~ziyeyang@134.134.137.71) [01:56:59] *** Quits: ziyeyang_ (~ziyeyang@134.134.137.71) (Ping timeout: 255 seconds) [01:57:04] *** Joins: ziyeyang__ (ziyeyang@nat/intel/x-tgjuqehjykpemcym) [02:02:41] *** Joins: ziyeyang_ (~ziyeyang@134.134.137.71) [02:02:41] *** Quits: ziyeyang__ (ziyeyang@nat/intel/x-tgjuqehjykpemcym) (Remote host closed the connection) [02:11:23] *** Quits: ziyeyang_ (~ziyeyang@134.134.137.71) (Ping timeout: 240 seconds) [07:09:10] *** Joins: johnmeneghini (~johnmeneg@216.240.30.5) [07:09:46] *** Quits: johnmeneghini (~johnmeneg@216.240.30.5) (Client Quit) [07:25:49] For anyone that might have missed the dist list email I just sent out... here's the link to the SPDK Developer Meetup in Nov, please RSVP soon if you can make it! http://evite.me/DTTUNyNGw4 [07:32:13] will get the info posted on the community tab on spdk.io as well here shortly... [08:28:18] *** Joins: lhodev (~Adium@inet-hqmc08-o.oracle.com) [08:35:28] Hey gang. I submitted my very first patch to the SPDK repo over the weekend. I did not explicitly add any reviewers; just want to verify this is proper protocol for SPDK. Also, I note on the GerritHub page with my change selected, toward the upper right corner it states "Conflicts With(1)" and below that is shown a change someone else did to the same source, albeit a while ago. I had done a rebase just prior to pushing my change so I' [08:35:28] m a little bewildered what this means. I'm assuming it's something to do with the fact that perhaps the "conflict" is with something that's been enqueued, but not yet accepted. I don't know what, if anything, I need to do at this point. [08:42:07] *** ChanServ sets mode: +o jimharris [08:44:48] hi lhodev - thanks for your patch! you can effectively ignore that conflict for now, you're right - Gerrit is just saying that your patch conflicts with another patch that had been enqueued previously but hasn't been committed yet [08:46:18] if you had not rebased, and your patch conflicted with master, you would see a "Cannot Merge" message on your patch [08:49:24] Thanks for letting me know. Also, using the GerritHub UI I added a couple of comments to offer more context on what I did. I note in the UI that this is marked as "Drafts". Normally, from a semantic standpoint, I interpret "Drafts" as something that has not been SAVED in which case others might not see it. I'm digging through the GerritHub online docs, and it seems that may not be the case. I admittedly can be a pedant and as I'm new [08:49:24] to the process I want to ensure I'm following the steps correctly. [08:59:41] Hi Lhodev, If I'm understanding you correctly, your comments have been saved, but not posted yet. [09:00:50] Once you write an inline comment on a patch, you can go back up to the homepage for your change and at the top of the screen there will be a blue button that says "reply..." click that, then a dropdown will appear, and you can press post from there and your comments will become visible to other contributors. [09:03:28] I think I did that correctly. Again, I may just be hung up on the appearance of the word "drafts: 1" in red under the Comments column in the Files section (bottom left) of the UI. I'm gathering that may be 'normal'; i.e. it does not indicate that my comments are invisible to others. Right? [09:07:27] I just pulled up your change. It looks like I can see your comment. The one that starts "I elected" and ends "this scenario occurs." [09:07:42] Right. [09:08:13] hmmm - no, if you see "drafts: 1" in red, that indicates the comments haven't been posted yet [09:09:05] Confused. sethhowe is able to see them. What, then, exactly, does "posting them" mean? And, how do I facilitate that? [09:11:25] if your browser is showing the most recent version of your patch (you can confirm by refreshing), and it still shows "drafts: 1", then there is one comment you've made that is not visible to anyone else yet [09:11:43] you can hit "Reply" at the top - it should show your comment, then you hit "Post" to post it [09:13:07] There we go…. That cleared up the issue. No longer "drafts: 1" in red, but now "comments: 1". Yay ;-) [09:15:46] :) [09:19:07] *** Quits: sethhowe (~sethhowe@134.134.139.82) (Remote host closed the connection) [09:19:24] *** Joins: sethhowe (~sethhowe@192.55.55.41) [09:33:18] anyone else having issues with GerritHub? [09:51:13] it was down for me earlier, but it seems to be working now [09:53:18] Heading out for a few to see if I can catch the eclipse. Partially cloudy here in Austin TX. Eclipse here is expected to be appx 65% occlusion. We'll see… [10:16:14] jimharris: looks like the blob readv/writev patch has a scan-build warning: https://review.gerrithub.io/#/c/374880/ [10:21:36] probably just needs a CU_ASSERT -> SPDK_CU_ASSERT_FATAL [10:57:11] jimharris, did you see my comments on that patch? mostly general questions but the last one I think might need a change [10:59:53] yes - I pushed a new rev earlier this morning [11:00:10] good feedback [11:00:46] peluse-: posted a comment on your developer meetup website patch [11:02:04] drv, thanks [11:04:43] drv, ya I can do that [11:13:18] jimharris: For this blobstore readv/writev patch. I made one comment, but in general I think it is fine [11:13:26] however, there is probably a faster way to do this [11:13:56] in the case where an I/O spans a cluster boundary, we need to allocate a new iov such that no individual element spans a cluster boundary [11:14:13] we can do that up front, synchronously, and only allocate one iovec array [11:14:25] then submit a batch of the I/Os in parallel [11:14:31] and when the whole batch completes, free the iovec array [11:15:11] and as a further optimization - don't allocate the new iovec array if the one the user provided already meets the requirement [11:18:00] nice [11:22:13] how do I hook a callback to free the iovec array? [11:22:51] there's this sequence_to_batch stuff to set a callback when a batch completes [11:23:54] I don't really have a mechanism built in to the sequence/batch stuff [11:24:09] but you can also allocate your own spdk_bs_cpl, pass that to the batch [11:24:16] ok - are you asking me to put that in and then rework the readv/writev patch? [11:24:27] and make it's cb_arg point at a data structure that holds the user cb stuff [11:24:40] I don't think we need to redo your patch now [11:24:41] drv, what's the deal with this img: SPDK_text_2.png that I see in a few posts, I don't see the .png file anywhere so not sure what it is or if I should include it? [11:24:48] besides the comment I made, it can go in [11:26:20] peluse-: you can leave that out - we don't actually display the image anymore [11:26:31] drv, roger [11:27:13] and you can probably set category to 'news' [11:27:41] we haven't been very consistent with the category so far... somehow the summit announcement got put into 'release' :) [11:28:42] yeah I saw that and did. just submitted [11:35:51] I think all of us missed an off-by-one error on the initial length/split check too [11:36:00] should be <= instead of < [11:36:04] * peluse- was off by 2 [11:37:14] *** Quits: peluse (~peluse@2600:8800:140a:ae18:7d39:65b8:3fa:4996) (Quit: Leaving...) [11:37:14] *** peluse- is now known as peluse [11:37:33] *** ChanServ sets mode: +o peluse [11:38:46] drv, hey when you get a chance (no hurry) can you remove the -1 label from https://review.gerrithub.io/#/c/372541/? The series is ready for review and that one failed for some unrelated reason and I don't want to repush and risk some other bizarre fallout :) [11:41:22] looks like soft-roce probably hit an error [11:41:29] I selected it to re-run [11:42:31] bwalker, thanks [12:02:17] drv, bwalker: can you take a look at https://review.gerrithub.io/#/c/374519/? darek has some patches in the queue that will be on top of this (knocking out items in the todo list in the accompanying readme) [12:09:51] holy big patches Batman! [12:30:26] Doh! Attacked by gremlins again... https://review.gerrithub.io/#/c/372541/ [12:32:56] bwalker: could you look at https://review.gerrithub.io/#/c/374182/ [12:43:00] shit, I don't need the include on that anymore, didn't catch that w/the last update. I'd be down with making the #define change if bwalker is cool with it. I'll go remove the include now [12:48:25] cool - then we just put your patch in as it is - we can do the SPDK_BS_PAGE_SIZE thing as a follow-up patch [12:49:48] wait, I do need it obviously unless I use spdk_bs_get_page_size() and also change it to not take a parm [12:50:18] it meaning the .h file include... [12:51:23] yeah, we should decide if the page size is just a universal constant or if it's still per-bs_dev [12:51:59] but that's a public API right? So probably not a good idea to just yank it. I also thought that at some point we wanted to maybe make page size configurable to something else [12:57:40] but I will remove the include and use the function call, I have a bs * at that point so it'll work... [12:59:00] oh never mind, I don't. so it's "as is" or something else :) [12:59:48] "as is" seems OK for now [12:59:53] and given that the file in question is part of the blbostore implementation, what's the main concern with including blobstore.h? [13:00:28] *** Quits: lhodev (~Adium@inet-hqmc08-o.oracle.com) (Quit: Leaving.) [13:37:14] *** Joins: lhodev (~Adium@inet-hqmc08-o.oracle.com) [13:42:28] because blobstore.h isn't part of the public API [13:42:43] we reserve the right to change things in blobstore.h without considering it an API break [13:43:17] but we probably do need to do the #define thing Jim mentioned as a follow up patch [13:44:06] or actually [13:44:12] here's a better thought [13:44:17] don't do the check in blob_bdev.c at all [13:44:21] do it in spdk_bs_init [13:44:55] then you don't have any issues [13:46:57] yup yup, good thought. will do [13:47:00] oh yeah, I think I commented on that previously, but I forgot about it [13:47:18] we should do it in bs_init anyway since other bs_dev implementations can exist [13:47:23] *** Parts: lhodev (~Adium@inet-hqmc08-o.oracle.com) () [14:05:12] peluse: can you tweak the link in your blog post so it is clickable? [14:05:26] you can see the output locally if you have jekyll installed, or on the build pool [14:05:28] http://spdk.intel.com/public/spdk.github.io.pub/builds/review/6f3785af15ddfca7513379d0f61553c793d42220.1503340090/spdk.intel.com/_site/news/2017/08/21/dev_meetup/ [14:05:43] also the header is probably redundant since the title is right above it [14:05:43] drv, yeah, will give it a shot [14:06:16] question back on the prev topic, in returning from bs_init() I need to pick an errno for the callback, any suggestions? [14:06:34] EINVAL is probably fine [14:06:36] EINVAL [14:06:44] cool [14:06:57] the catch-all "you messed up" error code :) [14:09:29] lol, yeah the error log has good info though [14:59:14] bwalker: are you ok with committing https://review.gerrithub.io/#/c/374519/? [17:32:39] jimharris, good catch on the UT thing with the blocklen patch. fixed. [17:33:57] looks good - +2 [17:41:51] thx, also did the #define thing as a, get this, dependent patch :) [17:44:17] * jimharris passes out [17:45:49] LOL, I probably screwed it up though! [17:51:08] drv, FYI I fixed up the blog post [17:51:19] cool, I'll check it out [17:54:04] peluse: looks like it got a new change-id somehow (the old one is still open too) [17:54:14] f&*(k [17:54:16] oh, I guess you just don't have the gerrit hooks installed for that repo [17:54:22] it doesn't have a Change-Id line at all [17:54:54] we should write that up in the docs, but it's the same .git/hooks/commit-msg as the main spdk repo [17:54:57] strange, same repo as the first one but this is only the 2nd time I've used that repo. I'll go double check all the settings [17:55:45] peluse: patch dependency looked good to me - posted a couple of comments [17:56:55] I guess we also need to hook up the spdk.github.io pool to publish its results on ci.spdk.io (currently only on the internal server) [17:57:02] ok, try that [17:59:20] jimharris, sure enough. Wrt removing the API though, that's a public API and granted there aren't a ton of people using it are we sure we just want to yank it? Also what about someday maybe wanting to have different page sizes? Might be better to leave it as is so we have the API exposed [18:04:01] jimharris, sorry - put my replies in GH instead... [18:22:39] *** Joins: lhodev (~Adium@inet-hqmc08-o.oracle.com) [18:38:28] *** Joins: ziyeyang_ (~ziyeyang@192.55.54.45) [19:27:46] *** Quits: lhodev (~Adium@inet-hqmc08-o.oracle.com) (Quit: Leaving.)