On 02/03/11 17:55, Paul Eggert wrote: > On 03/02/2011 04:53 AM, Pádraig Brady wrote: >> + /* Enable 'fullblock' with 'direct' or 'cio' as we don't want to >> + write partial blocks to output, which would disable O_DIRECT. An >> + alternative would be to enable C_TWOBUFS to accumulate full output >> + blocks. However that wouldn't work when a count is specified, and >> + is also less efficient. */ >> + if (output_flags & (O_DIRECT | O_CIO)) >> + input_flags |= O_FULLBLOCK; > > I'm afraid this patch feels wrong somehow. It's conflating four > issues: counting, efficiency, disabling O_DIRECT, and disabling O_CIO. > Some thoughts: > > 1. Offhand I don't see why O_CIO has anything to do with blocking; > why is it mentioned here?
Well in my quick googling it seemed to be a superset of O_DIRECT mode, with the same alignment constraints. It seems to fall back to a standard (synchronous) write if the alignment is not maintained (we should always be OK there), so I thought it safer to add it given its close relationship with O_DIRECT. I'll remove it until I can actually test this. > > 2. If C_TWOBUFS is already in effect, then dd needn't set > O_FULLBLOCK, since C_TWOBUFS already prevents the disabling of > O_DIRECT. True. But it shouldn't cause an issue. I'll leave as is to simplify the code and docs. I'll amend the comment. > 3. The counting issue is independent of oflag=direct or oflag=cio: > any solution for counting should work regardless of oflag settings. True. I'll amend the comment. > The more I think about it, the more I suspect that this > O_FULLBLOCK-inferring code should be omitted. It adds little benefit, as > it covers a rare combination of flags that is likely to be used only > by experts, who should know the gotchas in this area anyway. And the > extra complexity in documentation will penalize everybody who reads > it, even the non-experts. Well it has caught people out recently: https://bugzilla.redhat.com/show_bug.cgi?id=614605 https://lkml.org/lkml/2011/2/22/746 I'm still inclined to add this. Thanks for all the insightful comments on this BTW. dd really is hairy. Pádraig.
