Bug #3607
closedFileStore::_write conditional code for HAVE_SYNC_FILE_RANGE seems wrong
0%
Description
I won't claim to fully understand this, but it looks wrong:
// flush? if ((ssize_t)len < m_filestore_flush_min || #ifdef HAVE_SYNC_FILE_RANGE !m_filestore_flusher || !queue_flusher(fd, offset, len) #else true #endif ) { if (m_filestore_sync_flush) ::sync_file_range(fd, offset, len, SYNC_FILE_RANGE_WRITE); lfn_close(fd); }
Seems like the whole last if should be avoided in the #else case, at least.
Updated by Gerben Meijer over 11 years ago
Tracked this down to https://github.com/ceph/ceph/commit/1477ec73e354972664424b3d98d78a20c24a2ff4 but I still can't really make much of it. I think the whole
if (m_filestore_sync_flush)
::sync_file_range(fd, offset, len, SYNC_FILE_RANGE_WRITE);
needs to be dropped #ifdef HAVE_SYNC_FILE_RANGE
Updated by Sage Weil over 11 years ago
the if needs to go in the #ifdef, yeah.. but hte way it's split across the if () guts is messy. can you just clean it up into two simple cases.. the sync_file_range case where it does the if and tries to flush, and the else case that ust does lfn_close()? there will be two lfn_close calls in that case (both both #if branches), but it'll be easier to understand.
thanks!