> On Sep 11, 2014, at 1:48 PM, TR Reardon <thomas_reardon@xxxxxxxxxxx> wrote:
>> Picking this back up. How would O_TMPFILE avoid races? It definitely avoids the unwanted mtime/atime update, but then the existing "<filename>.defrag" pseudo-lock file would no longer be available. How could you use O_TMPFILE and still avoid multiple defrag? If this isn't possible, then resetting the parent times on unlink(tmpfile), as you suggest, is the simplest way out of this.
>
> Looking at this the opposite way - what are the chances that there
> will be concurrent defrags on the same file? Basically no chance at
> all. So long as it doesn't explode (the kernel would need to protect
> against this anyway to avoid malicious apps), the worst case is that
> there will be some extra defragmentation done in a very rare case.
>
> Conversely, creating a temporary filename and then resetting the
> parent directory timestamp is extra work for every file defragmented,
> and is racy because e4defrag may "reset" the time to before the temp
> file was created, but clobber a legitimate timestamp update in the
> directory from some other concurrent update. That timestamp update
> is always going to be racy, even if e4defrag tries to be careful.
>
> Cheers, Andreas
Thanks, well described.
So a simple attempt with O_TMPFILE first, then revert to original behavior, like below?
diff --git a/misc/e4defrag.c b/misc/e4defrag.c
index d0eac60..8001182 100644
--- a/misc/e4defrag.c
+++ b/misc/e4defrag.c
@@ -40,6 +40,7 @@
#include <sys/stat.h>
#include <sys/statfs.h>
#include <sys/vfs.h>
+#include <libgen.h>
/* A relatively new ioctl interface ... */
#ifndef EXT4_IOC_MOVE_EXT
@@ -1526,31 +1527,36 @@ static int file_defrag(const char *file, const struct stat64 *buf,
/* Create donor inode */
memset(tmp_inode_name, 0, PATH_MAX + 8);
- sprintf(tmp_inode_name, "%.*s.defrag",
- (int)strnlen(file, PATH_MAX), file);
- donor_fd = open64(tmp_inode_name, O_WRONLY | O_CREAT | O_EXCL, S_IRUSR);
+ /* Try O_TMPFILE first, to avoid changing directory mtime */
+ sprintf(tmp_inode_name, "%.*s", (int)strnlen(file, PATH_MAX), file);
+ donor_fd = open64(dirname(tmp_inode_name), O_TMPFILE | O_WRONLY | O_EXCL, S_IRUSR | S_IWUSR );
if (donor_fd < 0) {
- if (mode_flag & DETAIL) {
- PRINT_FILE_NAME(file);
- if (errno == EEXIST)
- PRINT_ERR_MSG_WITH_ERRNO(
- "File is being defraged by other program");
- else
- PRINT_ERR_MSG_WITH_ERRNO(NGMSG_FILE_OPEN);
+ sprintf(tmp_inode_name, "%.*s.defrag",
+ (int)strnlen(file, PATH_MAX), file);
+ donor_fd = open64(tmp_inode_name, O_WRONLY | O_CREAT | O_EXCL, S_IRUSR);
+ if (donor_fd < 0) {
+ if (mode_flag & DETAIL) {
+ PRINT_FILE_NAME(file);
+ if (errno == EEXIST)
+ PRINT_ERR_MSG_WITH_ERRNO(
+ "File is being defraged by other program");
+ else
+ PRINT_ERR_MSG_WITH_ERRNO(NGMSG_FILE_OPEN);
+ }
+ goto out;
}
- goto out;
- }
- /* Unlink donor inode */
- ret = unlink(tmp_inode_name);
- if (ret < 0) {
- if (mode_flag & DETAIL) {
- PRINT_FILE_NAME(file);
- PRINT_ERR_MSG_WITH_ERRNO("Failed to unlink");
+ /* Unlink donor inode */
+ ret = unlink(tmp_inode_name);
+ if (ret < 0) {
+ if (mode_flag & DETAIL) {
+ PRINT_FILE_NAME(file);
+ PRINT_ERR_MSG_WITH_ERRNO("Failed to unlink");
+ }
+ goto out;
}
- goto out;
}
-
+
/* Allocate space for donor inode */
orig_group_tmp = orig_group_head;
do {
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
