Skip to content

fix: race condition for device error propagation #620

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
madjesc wants to merge 2 commits into
base: release/v2.4.9.1
Choose a base branch
from
Open

fix: race condition for device error propagation #620

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
6a5b8bf
fix: race condition for device error propagation
Jun 8, 2026
57515dd
remove unnecesary
madjesc Aug 27, 2026
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
57 changes: 0 additions & 57 deletions src/iosched/unified.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,6 @@ void _unified_unset_write_ip(struct dentry_priv *dpr, struct unified_data *priv)
void _unified_handle_write_error(ssize_t write_ret, struct write_request *req,
struct dentry_priv *dpr, struct unified_data *priv);
int _unified_get_write_error(struct dentry_priv *dpr);
int _unified_write_index_after_perm(int write_ret, struct unified_data *priv);

/**
* Initialize an instance of the unified scheduler.
Expand Down Expand Up @@ -1189,9 +1188,6 @@ void _unified_process_index_queue(struct unified_data *priv)
if (ret < 0) {
/* Index partition writer: failed to write data to the tape (%d) */
ltfsmsg(LTFS_WARN, 13013W, (int)ret);
if (IS_WRITE_PERM(-ret)) {
ret = tape_set_cart_volume_lock_status(priv->vol, PWE_MAM_IP);

@syaoraang syaoraang Sep 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Since this is under the unified and the lock is now in the iosched, we'll lose those guards.
Is this intended?

}
_unified_handle_write_error(ret, req, dentry_priv, priv);
break;
} else {
Expand Down Expand Up @@ -1283,7 +1279,6 @@ void _unified_process_data_queue(enum request_state queue, struct unified_data *
if (ret < 0) {
/* Data partition writer: failed to write data to the tape (%d) */
ltfsmsg(LTFS_WARN, 13014W, (int)ret);
(void)_unified_write_index_after_perm(ret, priv);

@syaoraang syaoraang Sep 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Since this is under the unified and the catch of the perm is now in the iosched, we'll lose that behavior.
Is this intended?
This applies for next 3 changes

_unified_handle_write_error(ret, req, dentry_priv, priv);
break;
} else {
Expand Down Expand Up @@ -1314,7 +1309,6 @@ void _unified_process_data_queue(enum request_state queue, struct unified_data *
if (ret < 0) {
/* Data partition writer: failed to write data to the tape (%d) */
ltfsmsg(LTFS_WARN, 13014W, (int)ret);
(void)_unified_write_index_after_perm(ret, priv);
break;
} else {
TAILQ_REMOVE(&local_req_list, req, list);
Expand Down Expand Up @@ -1965,7 +1959,6 @@ int _unified_flush_unlocked(struct dentry *d, struct unified_data *priv)
ret = ltfs_fsraw_write(d, req_cache, req->count, req->offset, dp_id, false, priv->vol);
if (ret < 0) {
ltfsmsg(LTFS_ERR, 13019E, (int)ret);
(void)_unified_write_index_after_perm(ret, priv);
_unified_handle_write_error(ret, req, dpr, priv);
break;
} else if (dpr->write_ip) {
Expand Down Expand Up @@ -2250,56 +2243,6 @@ int _unified_get_write_error(struct dentry_priv *dpr)
return ret;
}

int _unified_write_index_after_perm(int write_ret, struct unified_data *priv)
{
int ret = 0;
struct tc_position err_pos;
uint64_t last_index_pos = UINT64_MAX;
unsigned long blocksize;

if (!IS_WRITE_PERM(-write_ret)) {
/* Nothing to do for non-medium error */
return ret;
}

ltfsmsg(LTFS_INFO, 13024I, write_ret);
ret = tape_set_cart_volume_lock_status(priv->vol, PWE_MAM_DP);
if (ret < 0)
ltfsmsg(LTFS_ERR, 13026E, "update MAM", ret);

blocksize = ltfs_get_blocksize(priv->vol);

ret = tape_get_first_untransfered_position(priv->vol->device, &err_pos);
if (ret < 0) {
ltfsmsg(LTFS_ERR, 13026E, "get error pos", ret);
return ret;
}

/* Check the err_pos is larger than the last index position of the partition */
if (err_pos.partition == ltfs_part_id2num(priv->vol->label->partid_ip, priv->vol)) {
last_index_pos = priv->vol->ip_coh.set_id;
} else {
last_index_pos = priv->vol->dp_coh.set_id;
}

if (last_index_pos > err_pos.block) {
ltfsmsg(LTFS_INFO, 13027I, (int)err_pos.partition,
(unsigned long long)err_pos.block, (unsigned long long)last_index_pos);
err_pos.block = last_index_pos + 1;
}

ltfsmsg(LTFS_INFO, 13025I, (int)err_pos.partition, (unsigned long long)err_pos.block, blocksize);
ret = ltfs_fsraw_cleanup_extent(priv->vol->index->root, err_pos, blocksize, priv->vol);
if (ret < 0) {
ltfsmsg(LTFS_ERR, 13026E, "extent cleanup", ret);
return ret;
}

ret = ltfs_write_index(ltfs_ip_id(priv->vol), SYNC_WRITE_PERM, priv->vol);

return ret;
}

/**
* Enable profiler function
* @param work_dir work directory to store profiler data
Expand Down
66 changes: 63 additions & 3 deletions src/libltfs/iosched.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,11 @@
**
*************************************************************************************
*/
#include "ltfs_error.h"

@syaoraang syaoraang Sep 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not needed header

#include "ltfs_fuse.h"
#include "ltfs_fsops_raw.h"
#include "ltfs.h"
#include "tape.h"
#include "iosched.h"

struct iosched_priv {
Expand All @@ -61,6 +64,53 @@ struct iosched_priv {
void *backend_handle; /**< Backend private data */
};

int _iosched_write_index_after_perm(int write_ret, struct ltfs_volume *volume)
{
int ret = 0;
struct tc_position err_pos;
uint64_t last_index_pos = UINT64_MAX;
unsigned long blocksize;

if (!IS_WRITE_PERM(-write_ret)) {

@Piloalucard Piloalucard Oct 1, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This check is unnecessary if this is going to be used like below, you do the check IS_WRITE_PERM and inside you call this function

/* Nothing to do for non-medium error */
return ret;
}

ltfsmsg(LTFS_INFO, 13024I, write_ret);
blocksize = ltfs_get_blocksize(volume);

ret = tape_get_first_untransfered_position(volume->device, &err_pos);
if (ret < 0) {
ltfsmsg(LTFS_ERR, 13026E, "get error pos", ret);
return ret;
}

/* Check the err_pos is larger than the last index position of the partition */
if (err_pos.partition == ltfs_part_id2num(volume->label->partid_ip, volume)) {
last_index_pos = volume->ip_coh.set_id;
} else {
last_index_pos = volume->dp_coh.set_id;
}

if (last_index_pos > err_pos.block) {
ltfsmsg(LTFS_INFO, 13027I, (int)err_pos.partition,
(unsigned long long)err_pos.block, (unsigned long long)last_index_pos);
err_pos.block = last_index_pos + 1;
}

ltfsmsg(LTFS_INFO, 13025I, (int)err_pos.partition, (unsigned long long)err_pos.block, blocksize);
ret = ltfs_fsraw_cleanup_extent(volume->index->root, err_pos, blocksize, volume);
if (ret < 0) {
ltfsmsg(LTFS_ERR, 13026E, "extent cleanup", ret);
return ret;
}

ret = ltfs_write_index(ltfs_ip_id(volume), SYNC_WRITE_PERM, volume);

return ret;
}


/**
* Initialize the I/O scheduler.
* @param plugin The plugin to take scheduler operations from.
Expand Down Expand Up @@ -230,10 +280,20 @@ ssize_t iosched_write(struct dentry *d, const char *buf, size_t size, off_t offs
CHECK_ARG_NULL(d, -LTFS_NULL_ARG);

ret = priv->ops->write(d, buf, size, offset, isupdatetime, priv->backend_handle);
if (ret > 0 && (size_t) ret > size)
ret = size;
if (ret < 0) {
if (IS_WRITE_PERM(-ret)) {
mam_lockval vollock = d->matches_name_criteria ? PWE_MAM_IP : PWE_MAM_DP;

@amissael95 amissael95 Jul 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fix indentation please.

Comment thread
madjesc marked this conversation as resolved.
int sync_ret = _iosched_write_index_after_perm(ret, vol);
if (sync_ret < 0) {
tape_set_cart_volume_lock_status(vol, PWE_MAM);
return sync_ret;
}
tape_set_cart_volume_lock_status(vol, vollock);
}
return ret;
}

return ret;
return ret > (ssize_t)size? (ssize_t)size : ret;

@amissael95 amissael95 Sep 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This was also done before this PR change in lines 233 - 334. It looks like it is done to ensure than the return value is always equal to the number of bytes (size variable) wanted to be written, this is in case unified_write returns more bytes than the bytes wanted to write because a "bug":

 * @param size Number of bytes to write.
 *[...]
 * @return Number of bytes written on success, or a negative value on error. The number of
 *         bytes written on success always equals 'size'; any other nonnegative return value from
 *         this function is a bug. In the future, the scheduler write interface may change to
 *         return 0 on success.
 */
ssize_t unified_write(struct dentry *d, const char *buf, size_t size, off_t offset,

Could you confirm it? Do you have more context of how that bug can be generated/triggered?

}

/**
Expand Down
38 changes: 6 additions & 32 deletions src/libltfs/ltfs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@

#include "fs.h"
#include "ltfs.h"
#include "ltfs_error.h"

@syaoraang syaoraang Sep 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not necessary headder add.

#include "ltfs_internal.h"
#include "libltfs/ltfslogging.h"
#include "ltfs_copyright.h"
Expand Down Expand Up @@ -2431,15 +2432,14 @@ size_t ltfs_max_cache_size(struct ltfs_volume *vol)
*/
int ltfs_write_index(char partition, char *reason, struct ltfs_volume *vol)
{
int ret, ret_mam;
int ret;
struct tape_offset old_selfptr, old_backptr;
struct ltfs_timespec modtime_old = { .tv_sec = 0, .tv_nsec = 0 };
bool generation_inc = false;
struct tc_position physical_selfptr, current_position;
char *cache_path_save = NULL;
bool write_perm = (strcmp(reason, SYNC_WRITE_PERM) == 0);
bool update_vollock = false;
int volstat = -1, new_volstat = 0;
int volstat = -1;
char *bc_print = NULL;
unsigned long long diff;

Expand Down Expand Up @@ -2567,13 +2567,13 @@ int ltfs_write_index(char partition, char *reason, struct ltfs_volume *vol)
return -1;
}

/* Prior to writing the index, compare the current location of the head position to the head location
/* Prior to writing the index, compare the current location of the head position to the head location
that is kept in the cache of ltfs (physical_selfptr). If they are different return error (-1) */
diff = ((unsigned long long)physical_selfptr.block - (unsigned long long)current_position.block);
if (diff) {
/* Position mismatch, diff not equal zero */
ltfsmsg(LTFS_INFO, 17293E, (unsigned long long)physical_selfptr.block, (unsigned long long)current_position.block);
return -1;
return -LTFS_INDEX_INVALID;

@syaoraang syaoraang Sep 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This return value doesn't match with LTFS_INDEX_INVALID definition, besides the comment for this part specifies to return -1, if we change this return value, we should update the comment

}

old_selfptr = vol->index->selfptr;
Expand All @@ -2594,9 +2594,6 @@ int ltfs_write_index(char partition, char *reason, struct ltfs_volume *vol)
vol->index->backptr = old_backptr;
vol->index->selfptr = old_selfptr;

if (IS_WRITE_PERM(-ret))
update_vollock = true;

goto out_write_perm;
}
}
Expand All @@ -2614,9 +2611,6 @@ int ltfs_write_index(char partition, char *reason, struct ltfs_volume *vol)
vol->index->backptr = old_backptr;
vol->index->selfptr = old_selfptr;

if (IS_WRITE_PERM(-ret))
update_vollock = true;

goto out_write_perm;
}

Expand All @@ -2631,9 +2625,6 @@ int ltfs_write_index(char partition, char *reason, struct ltfs_volume *vol)
vol->index->backptr = old_backptr;
vol->index->selfptr = old_selfptr;

if (IS_WRITE_PERM(-ret))
update_vollock = true;

goto out_write_perm;
}

Expand Down Expand Up @@ -2677,23 +2668,6 @@ int ltfs_write_index(char partition, char *reason, struct ltfs_volume *vol)
ltfs_mutex_unlock(&vol->device->read_only_flag_mutex);
}

if (update_vollock) {
Comment thread
syaoraang marked this conversation as resolved.
if (volstat == PWE_MAM_DP && partition == ltfs_ip_id(vol))
new_volstat = PWE_MAM_BOTH;
else if (volstat == PWE_MAM_IP && partition == ltfs_dp_id(vol))
new_volstat = PWE_MAM_BOTH;
else if (volstat == UNLOCKED_MAM && partition == ltfs_ip_id(vol))
new_volstat = PWE_MAM_IP;
else if (volstat == UNLOCKED_MAM && partition == ltfs_dp_id(vol))
new_volstat = PWE_MAM_DP;
Comment on lines -2681 to -2688

@syaoraang syaoraang Sep 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I see that the MAM status gotten from volstats is managed in iosched_write(), but this function (ltfs_write_index()) is also called from other functions such as fuse's mount and unmount and some others.
So, we're loosing this status' management and the possible lock.
Is this expected?


if (new_volstat) {
ret_mam = tape_set_cart_volume_lock_status(vol, new_volstat);
if (ret_mam)
ret = ret_mam;
}
}

return ret;
}

Expand Down Expand Up @@ -4443,7 +4417,7 @@ static int _ltfs_write_rao_file(char *file_path_org, unsigned char *buf, size_t
ltfsmsg(LTFS_ERR, 10001E, __FILE__);
return -LTFS_NO_MEMORY;
}

arch_open(&fd, path,
O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
SHARE_FLAG_DENYRW, PERMISSION_READWRITE);
Expand Down
1 change: 1 addition & 0 deletions src/libltfs/ltfs_error.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,7 @@
#define EDEV_MEDIUM_MAX 20399 /* Maximum medium error value */

#define IS_MEDIUM_ERROR(e) ((e>=EDEV_MEDIUM_MIN)&&(e<=EDEV_MEDIUM_MAX))
#define IS_RW_PERM(e) ((e==EDEV_RW_PERM)||(e==EDEV_READ_PERM)||(e==EDEV_WRITE_PERM))
#define IS_READ_PERM(e) ((e==EDEV_RW_PERM)||(e==EDEV_READ_PERM)||(e==EDEV_MEDIUM_FORMAT_CORRUPTED))
#define IS_WRITE_PERM(e) ((e==EDEV_RW_PERM)||(e==EDEV_WRITE_PERM)||(e==EDEV_MEDIUM_FORMAT_CORRUPTED))

Expand Down
6 changes: 2 additions & 4 deletions src/libltfs/ltfs_fsops.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
*/

#include "ltfs.h"
#include "ltfs_error.h"

@syaoraang syaoraang Sep 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not necessary header, please remove it.

#include "ltfs_internal.h"
#include "ltfs_fsops.h"
#include "ltfs_fsops_raw.h"
Expand Down Expand Up @@ -1781,10 +1782,7 @@ int ltfs_fsops_write(struct dentry *d, const char *buf, size_t count, off_t offs
}
}

if (ret < 0)
return ret;
else
return 0;
return ret < 0? ret : 0;
Comment on lines -1784 to +1785

@syaoraang syaoraang Sep 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Works, but it's not necessary to change it since it's already working and this does not changes the behavior.

}

ssize_t ltfs_fsops_read(struct dentry *d, char *buf, size_t count, off_t offset,
Expand Down
6 changes: 3 additions & 3 deletions src/tape_drivers/generic/file/filedebug_tc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -880,9 +880,9 @@ int filedebug_write(void *device, const char *buf, size_t count, struct tc_posit
if ( state->write_counter > state->force_writeperm ) {
ltfsmsg(LTFS_ERR, 30007E, "write");
if (state->force_errortype)
return -EDEV_NO_SENSE;
return -EDEV_WRITE_PERM;
else
return -EDEV_WRITE_PERM;
return -EDEV_NO_SENSE;
Comment on lines -883 to +885

@syaoraang syaoraang Sep 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice catch!
But please fix indentation

} else if ( state->write_counter > (state->force_writeperm - THRESHOLD_FORCE_WRITE_NO_WRITE) ) {
ltfsmsg(LTFS_INFO, 30019I);
++state->current_position.block;
Expand Down Expand Up @@ -1426,7 +1426,7 @@ static inline int _sanitize_tape(struct filedebug_data *state)
ret = -EDEV_MEDIUM_FORMAT_ERROR;
break;
}
}
}
else if (gen == DRIVE_GEN_JAG4) {
switch (state->conf.cart_type) {
case TC_MP_JB:
Expand Down
Loading