On Tue, Feb 02, 2016 at 12:20:46PM +0100, Christoph Hellwig wrote: > On Fri, Jan 29, 2016 at 09:12:33AM -0500, Brian Foster wrote: > > My understanding is that the original requirement for ioends here was to > > track the state necessary in order to defer (to wq) completions that had > > to allocate transactions. Eventually, the deferred buffer state was > > implemented and we no longer required an ioend for that, so we removed > > the ioends here: > > > > 2ba6623 xfs: don't allocate an ioend for direct I/O completions > > > > Then just a couple months later, we merged the following to re-add the > > ioend into the dio path: > > > > d5cc2e3f xfs: DIO needs an ioend for writes > > And I complained about that one! I didn't have time to provide a full > analysis and the patches around this one were criticial enough. But this > patch has the analysis on why ioends are not needed and bad which should > have been the full review back then. Note that this new version is > better than the old ioend removal as it does not just use the private > data as a boolean, but as a bitmask so that additional information > such as the COW status can be communicated. > FWIW, I don't see any such review comments against the three versions of the "DIO needs an ioend for writes" patch I have in my mailbox, but I easily could have missed something..? But if there wasn't time, then fair enough. I'm just looking for context. I don't have much of an opinion on which approach is used here. If it simplifies COW, then that seems good enough reason to me to take this approach. I'm pointing this out more because this code seems to have been rewritten the last couple of times we needed to fix something, which makes backports particularly annoying. The two patches above were associated with a broader enhancement and a bug fix (respectively) as a sort of justification, whereas this post had a much more vague purpose from what I could tell, and therefore why I at least hadn't taken the time to review it. If COW is the primary motivator, perhaps we can bundle it with that work? Brian > _______________________________________________ > xfs mailing list > xfs@xxxxxxxxxxx > http://oss.sgi.com/mailman/listinfo/xfs _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
