Skip to content

Fix fsync absence when fsync=N parameter specified #1799

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix fsync absence when fsync=N parameter specified #1799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bdb3d50
io_u: make should_fsync() take a const
RomanSofyin Aug 25, 2025
7b7f18c
io_u/backend: fix missing final fsync() when using fsync=N
RomanSofyin Sep 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions backend.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1000,7 +1000,8 @@ static void do_io(struct thread_data *td, uint64_t *bytes_done)

while ((td->o.read_iolog_file && !flist_empty(&td->io_log_list)) ||
(!flist_empty(&td->trim_list)) || !io_issue_bytes_exceeded(td) ||
td->o.time_based) {
td->o.time_based ||
(should_fsync(td) && ddir_is_sync(td))) {
struct timespec comp_time;
struct io_u *io_u;
int full;
Expand Down Expand Up @@ -1032,8 +1033,8 @@ static void do_io(struct thread_data *td, uint64_t *bytes_done)
*/
if (bytes_issued >= total_bytes &&
!td->o.read_iolog_file &&
(!td->o.time_based ||
(td->o.time_based && td->o.verify != VERIFY_NONE)))
((!td->o.time_based && !(should_fsync(td) && ddir_is_sync(td))) ||
(td->o.time_based && td->o.verify != VERIFY_NONE)))
break;

io_u = get_io_u(td);
Expand Down
2 changes: 1 addition & 1 deletion fio.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,7 +628,7 @@ static inline bool multi_range_trim(struct thread_data *td, struct io_u *io_u)
return false;
}

static inline bool should_fsync(struct thread_data *td)
static inline bool should_fsync(const struct thread_data *td)
{
if (ddir_sync(td->last_ddir_issued))
return false;
Expand Down
37 changes: 25 additions & 12 deletions io_u.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -742,19 +742,8 @@ static enum fio_ddir rate_ddir(struct thread_data *td, enum fio_ddir ddir)
return ddir;
}

/*
* Return the data direction for the next io_u. If the job is a
* mixed read/write workload, check the rwmix cycle and switch if
* necessary.
*/
static enum fio_ddir get_rw_ddir(struct thread_data *td)
static inline enum fio_ddir get_sync_ddir(const struct thread_data *td)
{
enum fio_ddir ddir;

/*
* See if it's time to fsync/fdatasync/sync_file_range first,
* and if not then move on to check regular I/Os.
*/
if (should_fsync(td) && td->last_ddir_issued == DDIR_WRITE) {
if (td->o.fsync_blocks && td->io_issues[DDIR_WRITE] &&
!(td->io_issues[DDIR_WRITE] % td->o.fsync_blocks))
Expand All @@ -768,6 +757,30 @@ static enum fio_ddir get_rw_ddir(struct thread_data *td)
!(td->io_issues[DDIR_WRITE] % td->sync_file_range_nr))
return DDIR_SYNC_FILE_RANGE;
}
return DDIR_INVAL;
}

bool ddir_is_sync(const struct thread_data *td)
{
return get_sync_ddir(td) != DDIR_INVAL;
}

/*
* Return the data direction for the next io_u. If the job is a
* mixed read/write workload, check the rwmix cycle and switch if
* necessary.
*/
static enum fio_ddir get_rw_ddir(struct thread_data *td)
{
enum fio_ddir ddir;

/*
* See if it's time to fsync/fdatasync/sync_file_range first,
* and if not then move on to check regular I/Os.
*/
ddir = get_sync_ddir(td);
if (ddir != DDIR_INVAL)
return ddir;

if (td_rw(td)) {
/*
Expand Down
2 changes: 2 additions & 0 deletions io_u.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,8 @@ bool queue_full(const struct thread_data *);
int do_io_u_sync(const struct thread_data *, struct io_u *);
int do_io_u_trim(struct thread_data *, struct io_u *);

bool ddir_is_sync(const struct thread_data *);
Copy link
Owner

@axboe axboe Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd personally make that static inline, and require that get_sync_ddir() is called only if should_fsync() is true already. That skips a function call in the hot issue path loop for each IO, even if fsync hasn't been set at all.

Copy link
Author

@RomanSofyin RomanSofyin Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you suggested I could call get_sync_ddir() only if should_fsync() is true:

bool ddir_is_sync(const struct thread_data *td)
{
	return should_fsync(td) && get_sync_ddir(td) != DDIR_INVAL;
}

But I don't quite get how you want me to make ddir_is_sync static. Since ddir_is_sync gets called from backend.c it needs to be either in backend.c or in io_u.h to have it static. But ddir_is_sync calls should_fsync and get_sync_ddir which are static as well so can't be referred from anywhere but io_u.c.

Can you please clarify this?

Copy link
Collaborator

@sitsofe sitsofe Aug 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RomanSofyin if you make ddir_is_sync static inline the whole definition can live in the io_u.h header (rather than being in io_u.c) and the whole thing will be copied inline into every place it used (see dprint_io_u() in io_u.h as an example).

Scratch that - your point about about not being able to see should_fsync and get_sync_ddir from the io_u.h header stands.

Copy link
Collaborator

@sitsofe sitsofe Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RomanSofyin I've chatted to @axboe and it should be sufficient to guard the call sites to ddir_is_sync() by a should_fsync() pre-check.

Copy link
Author

@RomanSofyin RomanSofyin Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was also thinking if it should be a bit nicer condition being wrapped in a macro or inline function. I mean both components together: (should_fsync(td) && ddir_is_sync(td)).


#ifdef FIO_INC_DEBUG
static inline void dprint_io_u(struct io_u *io_u, const char *p)
{
Expand Down