<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
</head>
<body>
Neil,<br>
<br>
Compute vs consume: Honestly either would work. The client consumes grant when it writes data, this fix to the calculation allows it to consume more accurately. *shrugs*<br>
<br>
- Patrick<br>
<br>
<br>
<hr style="display:inline-block;width:98%" tabindex="-1">
<div id="divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" style="font-size:11pt" color="#000000"><b>From:</b> lustre-devel <lustre-devel-bounces@lists.lustre.org> on behalf of NeilBrown <neilb@suse.com><br>
<b>Sent:</b> Monday, July 2, 2018 10:04:48 PM<br>
<b>To:</b> James Simmons; Andreas Dilger; Oleg Drokin<br>
<b>Cc:</b> Johann Lombardi; Lustre Development List<br>
<b>Subject:</b> Re: [lustre-devel] [PATCH 01/18] lustre: grant: add support for OBD_CONNECT_GRANT_PARAM</font>
<div> </div>
</div>
<div class="BodyFragment"><font size="2"><span style="font-size:11pt;">
<div class="PlainText">On Mon, Jul 02 2018, James Simmons wrote:<br>
<br>
> From: Johann Lombardi <jlombardi@whamcloud.com><br>
><br>
> Add support for grant overhead calculation on the client side.<br>
> To do so, clients track usage on a per-extent basis. An extent is<br>
> composed of contiguous blocks.<br>
> The OST now returns to the OSC layer several parameters to consume<br>
> grant more accurately:<br>
<br>
Just to be sure that I'm understanding correctly, should "consume"<br>
above be "compute" ??<br>
<br>
><br>
> - the backend filesystem block size which is the minimal grant<br>
> allocation unit;<br>
> - the maximum extent size;<br>
> - the extent insertion cost.<br>
> Clients now pack in bulk write how much grant space was consumed for<br>
> the RPC. Dirty data accounting also adopts the same scheme.<br>
><br>
> Moreover, each backend OSD now reports its own set of parameters:<br>
> - For ldiskfs, we usually have a 4KB block size with a maximum extent<br>
> size of 32MB (theoretical limit of 128MB) and an extent insertion<br>
> cost of 6 x 4KB = 24KB<br>
> - For ZFS, we report a block size of 128KB, an extent size of 128<br>
> blocks (i.e. 16MB with 128KB block size) and a block insertion cost<br>
> of 112KB.<br>
><br>
> Besides, there is now no more generic metadata overhead reservation<br>
> done inside each OSD. Instead grant space is inflated for clients<br>
> that do not support the new grant parameters. That said, a tiny<br>
> percentage (typically 0.76%) of the free space is still reserved<br>
> inside each OSD to avoid fragmentation which might hurt performance<br>
> and impact our grant calculation (e.g. extents are broken due to<br>
> fragmentation).<br>
><br>
> This patch also fixes several other issues:<br>
><br>
> - Bulk write resent by ptlrpc after reconnection could trigger<br>
> spurious error messages related to broken dirty accounting.<br>
> The issue was that oa_dirty is discarded for resent requests<br>
> (grant flag cleared in ost_brw_write()), so we can legitimately<br>
> have grant > fed_dirty in ofd_grant_check().<br>
> This was fixed by resetting fed_dirty on reconnection and skipping<br>
> the dirty accounting check in ofd_grant_check() in the case of<br>
> ptlrpc resend.<br>
<br>
ofd_grant_check doesn't appear in this patch. Presumably it was part of<br>
the server code that was stripped from the patch.<br>
<br>
><br>
> - In obd_connect_data_seqprint(), the connection flags cannot fit<br>
> in a 32-bit integer.<br>
<br>
I cannot see what this might refer to either.<br>
<br>
><br>
> - When merging two OSC extents, an extent tax should be released<br>
> in both the merged extent and in the grant accounting.<br>
<br>
I think I might see what this relates to.<br>
<br>
> +static ssize_t cur_dirty_grant_bytes_show(struct kobject *kobj,<br>
> + struct attribute *attr,<br>
> + char *buf)<br>
> +{<br>
> + struct obd_device *dev = container_of(kobj, struct obd_device,<br>
> + obd_kobj);<br>
> + struct client_obd *cli = &dev->u.cli;<br>
> + int len;<br>
> +<br>
> + spin_lock(&cli->cl_loi_list_lock);<br>
> + len = sprintf(buf, "%lu\n", cli->cl_dirty_grant);<br>
> + spin_unlock(&cli->cl_loi_list_lock);<br>
<br>
Why take the spinlock?? cl_dirty_grant is 'long', so reading it is<br>
atomic.<br>
<br>
<br>
> + if (OCD_HAS_FLAG(&cli->cl_import->imp_connect_data,<br>
> + GRANT_PARAM)) {<br>
> + int nrextents;<br>
> +<br>
> + /*<br>
> + * take extent tax into account when asking for more<br>
> + * grant space<br>
> + */<br>
> + nrextents = (nrpages + cli->cl_max_extent_pages - 1) /<br>
> + cli->cl_max_extent_pages;<br>
<br>
I generally prefer DIV_ROUND_UP() for this.<br>
<br>
<br>
I don't think any of the above will stop me from applying the patch,<br>
though I might edit the description, and may add a fix-up patch afterwards.<br>
<br>
Thanks.<br>
NeilBrown<br>
</div>
</span></font></div>
</body>
</html>