[03:56:14] *** Quits: gila (~gila@5ED4FE92.cm-7-5d.dynamic.ziggo.nl) (Quit: Textual IRC Client: www.textualapp.com) [05:45:41] Could you take a look at patch 370820? Now, after release do you think we could merge it? [07:12:25] *** Quits: tomzawadzki (~tzawadzk@134.134.139.73) (Ping timeout: 246 seconds) [09:14:55] sure [09:15:38] wrong window but I'm sure someone would respond that way anyways :) [11:30:15] i think we need two separate functions for splitting iov-based requests [11:30:25] currently we have _nvme_ns_cmd_split_sgl_request [11:30:44] this one is really meant for splitting iov requests on controllers that do not support SGL (i.e. they use PRP) [11:31:14] we want a separate one for splitting iov requests on controllers that do support SGL [11:31:22] this one will be a lot simpler [11:31:47] that seems reasonable to me [12:22:12] need an opinion on this sgl splitting change - so I can certainly make a separate function, but it turns out there will be a lot of duplicated code between the two variants (split SGL request based on PRP restrictions v. split SGL request based on SGL/max_sge restrictions) [12:22:43] I'm still leaning towards the separate function, since the existing function is pretty complicated already [12:31:38] you can duplicate, then see if the common code can get pulled into utility functions [12:31:42] that's always one strategy [12:32:10] except that the utility function will take 15 parameters at least [12:32:23] otherwise that's exactly what I'd do [12:32:39] then they're probably different enough to warrant duplicates [12:33:07] the common code would need to call _nvme_add_child_request which itself takes 15 parameters [12:55:08] *** Quits: guerby (~guerby@april/board/guerby) (Ping timeout: 240 seconds) [13:15:40] https://review.gerrithub.io/#/c/372359/ [13:15:58] passes all of our tests (so far) on sergio's system [13:16:07] for SGE splitting w/ NVMe-oF [13:35:30] jimharris: I made one comment, looks fine otherwise [13:35:51] definitely much simpler than trying to fit it into the existing split function [13:47:17] thanks - yeah, once I stared at it some more, I was able to make it quite a bit simpler than I was originally thinking [13:47:41] update patch pushed (and added similar error message in the existing prp split function) [13:49:12] cool, thanks [13:52:42] pushed one more change to the comment on the last patch - it was referring to child_equals_parent which is not a variable in the new function [14:00:26] hey what's the right terminology for env_spdk.cc? Is it like a shim or a module or a plug in or what? And if I draw a RocksDB box on top of a BlobFS box do I draw like a small sub-box inside the RocksDB box to represent the module-thingy-whatever? [14:09:34] RocksDB calls it an "Env" [14:09:42] thanks [14:10:10] RocksDB does all file operations through the Env abstraction [14:10:27] the default implementation of this abstraction is PosixEnv which routes them to standard lib calls [14:10:38] env_spdk.cc implements SpdkEnv [14:10:59] coolio [14:35:30] *** Joins: guerby (~guerby@april/board/guerby) [14:59:13] bwalker: gave some feedback on your bdev fio plugin patch - overall looks pretty good [14:59:19] I think we can push the four commits before it [14:59:25] you'll need to rebase the rest of the series anyways [15:06:08] peluse: is there a consolidated series of your nvme ut patches? [15:07:12] jimharris, unfortunately not. I screwed up the series as I was learning how to do it so dependencies exist that don't make sense (and weren't really required anyway) [15:08:02] dependencies between the patches weren't required? [15:08:40] technically no, I'm mean they're related but I'm pretty sure most of them didn't "need" earlier ones (some did of course but those I think are already merged) [15:09:30] it's more a git merge conflict issue [15:09:32] the right way would have been to have them all dependent on the one mock lib patch. Put instead I build them up dependent on the previous [15:09:58] meaning the previous that I happened to write but it was just another test_function. [15:10:04] so yea, they're kind messed up like that, sorry [15:10:36] and to make it more confusing, I didn't even get that scheme totally right [15:10:42] but on the + side I know what I'm dong now :) [15:10:58] it will end up being more work for you in a lot of cases - i.e. patch A and B both touch file X - we commit patch A, you'll need to rebase patch B and send it out for review again [15:11:16] yeah, yeah I've already experienced that. at least they're all small! [15:11:32] seriously - come into the office and I'll walk you through stacked git [15:12:08] won't make that mistake again... I'm pretty sure I can do it now w/o any trouble using straight up git. [15:17:45] drv: what's the difference between spdk_crc32c_init/update and spdk_crc32_ieee_init/update? [15:18:08] oh - never mind [15:18:27] I missed CRC32C v. CRC32 in the polynomial name [15:19:42] yeah, it's a little bit tricky - I thought about naming it crc32_iscsi or something [15:26:38] drv, OK I fixed https://review.gerrithub.io/#/c/369052/ and will hold off on the other 3 similar tests until you take a look at this one. Thanks! [15:27:53] drv, no big hurry of course... [15:28:44] peluse: I'll check it out [16:55:13] peluse: I posted some comments - I didn't give it a + or - since I want to hear what jimharris and bwalker think [17:14:44] cool [17:18:44] drv, just looked, yeah no problem. functionally the same and I don't think either is any clearer but your suggestion is less code so I'm in! Not tonight though, will update in the morn [18:17:53] *** Joins: ziyeyang_ (~ziyeyang@134.134.137.71) [19:41:04] *** Quits: whitepa (~whitepa@2601:601:1200:f23b:b1bd:1de0:67e6:111d) (Ping timeout: 246 seconds) [20:54:29] *** Quits: guerby (~guerby@april/board/guerby) (Read error: Connection reset by peer) [20:54:41] *** Joins: guerby (~guerby@april/board/guerby) [21:57:44] *** Quits: ziyeyang_ (~ziyeyang@134.134.137.71) (Remote host closed the connection) [23:09:56] *** Joins: tomzawadzki (~tzawadzk@192.55.54.42)