Bug #3607: FileStore::_write conditional code for HAVE_SYNC_FILE_RANGE seems wrong - Ceph - Ceph

Actions

Copy link

Bug #3607

closed

FileStore::_write conditional code for HAVE_SYNC_FILE_RANGE seems wrong

Added by Dan Mick over 11 years ago. Updated almost 11 years ago.

Status:

Resolved

Priority:

Normal

Assignee:

Category:

Target version:

% Done:

Source:

Development

Tags:

Backport:

Regression:

Severity:

Reviewed:

Affected Versions:

ceph-qa-suite:

Pull request ID:

Crash signature (v1):

Crash signature (v2):

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.

Actions

Copy link

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

Actions

Copy link

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!

Actions

Copy link