continues

Author: TingDaoK

include/aws/s3/private/s3_meta_request_impl.h

@@ -319,8 +319,6 @@ struct aws_s3_meta_request {
     bool recv_file_delete_on_failure;
     /* When true, use O_DIRECT for writing received data to file */
     bool recv_file_direct_io;
-    /* Base file position for O_DIRECT writes (from recv_file_position option) */
-    uint64_t recv_file_base_position;
 
     /* File I/O options. */
     struct aws_s3_file_io_options fio_opts;

include/aws/s3/s3_client.h

@@ -350,7 +350,9 @@ struct aws_s3_file_io_options {
      * Notes:
      * - Only supported on linux for now.
      * - Supported for both upload (send_filepath) and download (recv_filepath).
-     * - For download, O_DIRECT is not supported with AWS_S3_RECV_FILE_CREATE_OR_APPEND.
+     * - For download, O_DIRECT is only supported with AWS_S3_RECV_FILE_CREATE_OR_REPLACE
+     *   and AWS_S3_RECV_FILE_CREATE_NEW (i.e. writing from the beginning of the file).
+     *   APPEND and WRITE_TO_POSITION are not supported with O_DIRECT.
      * - Check NOTES for O_DIRECT for additional info https://man7.org/linux/man-pages/man2/openat.2.html
      * In summary, O_DIRECT is a potentially powerful tool that should be used with caution.
      */

source/s3_meta_request.c

@@ -290,10 +290,10 @@ int aws_s3_meta_request_init_base(
         meta_request->recv_file_delete_on_failure = options->recv_file_delete_on_failure;
 
         /* When direct_io is enabled, use O_DIRECT fd-based writes instead of FILE* fwrite.
-         * Supported for CREATE_OR_REPLACE, CREATE_NEW, and WRITE_TO_POSITION.
-         * APPEND is incompatible with O_DIRECT (no offset-based writes). */
+         * Only supported for CREATE_OR_REPLACE and CREATE_NEW (writing from the beginning). */
         if (meta_request->fio_opts.direct_io &&
-            options->recv_file_option != AWS_S3_RECV_FILE_CREATE_OR_APPEND) {
+            (options->recv_file_option == AWS_S3_RECV_FILE_CREATE_OR_REPLACE ||
+             options->recv_file_option == AWS_S3_RECV_FILE_CREATE_NEW)) {
 
             /* Validate preconditions same as the FILE* path */
             if (options->recv_file_option == AWS_S3_RECV_FILE_CREATE_NEW &&
@@ -305,26 +305,23 @@ int aws_s3_meta_request_init_base(
                 aws_raise_error(AWS_ERROR_S3_RECV_FILE_ALREADY_EXISTS);
                 goto error;
             }
-            if (options->recv_file_option == AWS_S3_RECV_FILE_WRITE_TO_POSITION &&
-                !aws_path_exists(meta_request->recv_filepath)) {
-                AWS_LOGF_ERROR(
-                    AWS_LS_S3_META_REQUEST,
-                    "id=%p Cannot receive file via WRITE_TO_POSITION: file not found.",
-                    (void *)meta_request);
-                aws_raise_error(AWS_ERROR_S3_RECV_FILE_NOT_FOUND);
-                goto error;
-            }
 
             meta_request->recv_file_direct_io = true;
-            meta_request->recv_file_base_position =
-                (options->recv_file_option == AWS_S3_RECV_FILE_WRITE_TO_POSITION) ? options->recv_file_position : 0;
 
             AWS_LOGF_DEBUG(
                 AWS_LS_S3_META_REQUEST,
-                "id=%p: O_DIRECT enabled for download write path. base_position:%" PRIu64,
-                (void *)meta_request,
-                meta_request->recv_file_base_position);
+                "id=%p: O_DIRECT enabled for download write path.",
+                (void *)meta_request);
         } else {
+            /* Fail if user requested O_DIRECT with APPEND or WRITE_TO_POSITION */
+            if (meta_request->fio_opts.direct_io) {
+                AWS_LOGF_ERROR(
+                    AWS_LS_S3_META_REQUEST,
+                    "id=%p O_DIRECT for download is only supported with CREATE_OR_REPLACE and CREATE_NEW",
+                    (void *)meta_request);
+                aws_raise_error(AWS_ERROR_UNSUPPORTED_OPERATION);
+                goto error;
+            }
             /* Standard FILE* path */
             switch (options->recv_file_option) {
                 case AWS_S3_RECV_FILE_CREATE_OR_REPLACE:
@@ -2185,29 +2182,26 @@ static void s_s3_meta_request_event_delivery_task(struct aws_task *task, void *a
 
                         if (meta_request->recv_file_direct_io) {
                             /* O_DIRECT write path — use offset-based direct I/O */
-                            uint64_t write_offset =
-                                meta_request->recv_file_base_position + delivery_range_start;
+                            uint64_t write_offset = delivery_range_start;
                             struct aws_byte_cursor write_cursor =
                                 aws_byte_cursor_from_array(response_body.ptr, response_body.len);
                             if (aws_file_path_write_to_offset_direct_io(
                                     meta_request->recv_filepath, write_offset, write_cursor)) {
                                 if (aws_last_error() == AWS_ERROR_UNSUPPORTED_OPERATION) {
-                                    /* O_DIRECT not supported, fall back to FILE* for remainder */
+                                    /* Platform doesn't support O_DIRECT, fall back to buffered I/O */
                                     AWS_LOGF_WARN(
                                         AWS_LS_S3_META_REQUEST,
-                                        "id=%p: O_DIRECT write not supported, falling back to buffered I/O",
+                                        "id=%p: O_DIRECT write not supported on this platform, "
+                                        "falling back to buffered I/O",
                                         (void *)meta_request);
                                     meta_request->recv_file_direct_io = false;
                                     aws_reset_error();
-                                    /* Open FILE* and seek to current position */
+                                    /* Open FILE* and write this chunk */
                                     meta_request->recv_file = aws_fopen(
                                         aws_string_c_str(meta_request->recv_filepath), "r+");
                                     if (meta_request->recv_file &&
-                                        aws_fseek(
-                                            meta_request->recv_file,
-                                            (int64_t)(meta_request->recv_file_base_position + delivery_range_start),
-                                            SEEK_SET) == AWS_OP_SUCCESS) {
-                                        /* Retry this write with fwrite */
+                                        aws_fseek(meta_request->recv_file, (int64_t)write_offset, SEEK_SET) ==
+                                            AWS_OP_SUCCESS) {
                                         if (fwrite(
                                                 (void *)response_body.ptr,
                                                 response_body.len,
@@ -2222,14 +2216,8 @@ static void s_s3_meta_request_event_delivery_task(struct aws_task *task, void *a
                                     } else {
                                         error_code = aws_last_error();
                                     }
-                                    if (error_code != AWS_ERROR_SUCCESS) {
-                                        AWS_LOGF_ERROR(
-                                            AWS_LS_S3_META_REQUEST,
-                                            "id=%p Failed O_DIRECT fallback to buffered write. aws-error:%s",
-                                            (void *)meta_request,
-                                            aws_error_name(error_code));
-                                    }
                                 } else {
+                                    /* Real I/O error — hard fail */
                                     error_code = aws_last_error();
                                     AWS_LOGF_ERROR(
                                         AWS_LS_S3_META_REQUEST,

tests/CMakeLists.txt

@@ -79,7 +79,9 @@ add_net_test_case(test_s3_get_object_file_path_create_new)
 add_net_test_case(test_s3_get_object_file_path_append)
 add_net_test_case(test_s3_get_object_file_path_to_position)
 add_net_test_case(test_s3_get_object_file_path_direct_io)
-add_net_test_case(test_s3_get_object_file_path_direct_io_to_position)
+add_net_test_case(test_s3_get_object_file_path_direct_io_content_verify)
+add_net_test_case(test_s3_get_object_file_path_direct_io_unsupported_append)
+add_net_test_case(test_s3_get_object_file_path_direct_io_unsupported_write_to_position)
 add_net_test_case(test_s3_get_object_empty_object)
 add_net_test_case(test_s3_get_object_multiple)
 add_net_test_case(test_s3_get_object_multiple_serial)

tests/s3_data_plane_tests.c

@@ -5,6 +5,7 @@
 
 #include "aws/s3/private/s3_checksums.h"
 #include "aws/s3/private/s3_client_impl.h"
+#include <aws/checksums/crc.h>
 #include "aws/s3/private/s3_default_buffer_pool.h"
 #include "aws/s3/private/s3_meta_request_impl.h"
 #include "aws/s3/private/s3_util.h"
@@ -28,6 +29,12 @@
 #include <aws/testing/stream_tester.h>
 #include <inttypes.h>
 
+/* Forward declaration for checksum validation callback used in O_DIRECT tests */
+void s_s3_test_validate_checksum(
+    struct aws_s3_meta_request *meta_request,
+    const struct aws_s3_meta_request_result *result,
+    void *user_data);
+
 AWS_TEST_CASE(test_s3_client_create_destroy, s_test_s3_client_create_destroy)
 static int s_test_s3_client_create_destroy(struct aws_allocator *allocator, void *ctx) {
     (void)ctx;
@@ -1688,21 +1695,122 @@ static int s_test_s3_get_object_file_path_direct_io(struct aws_allocator *alloca
 }
 
 AWS_TEST_CASE(
-    test_s3_get_object_file_path_direct_io_to_position,
-    s_test_s3_get_object_file_path_direct_io_to_position)
-static int s_test_s3_get_object_file_path_direct_io_to_position(struct aws_allocator *allocator, void *ctx) {
+    test_s3_get_object_file_path_direct_io_content_verify,
+    s_test_s3_get_object_file_path_direct_io_content_verify)
+static int s_test_s3_get_object_file_path_direct_io_content_verify(struct aws_allocator *allocator, void *ctx) {
     (void)ctx;
 
     struct aws_s3_tester tester;
-    AWS_ZERO_STRUCT(tester);
     ASSERT_SUCCESS(aws_s3_tester_init(allocator, &tester));
 
     struct aws_s3_tester_client_options client_options = {
         .part_size = MB_TO_BYTES(5),
     };
 
+    struct aws_s3_client *client = NULL;
+    ASSERT_SUCCESS(aws_s3_tester_client_new(&tester, &client_options, &client));
+
+    /* Upload with CRC32 checksum */
+    struct aws_byte_buf path_buf;
+    AWS_ZERO_STRUCT(path_buf);
+    ASSERT_SUCCESS(aws_s3_tester_upload_file_path_init(
+        allocator, &path_buf, aws_byte_cursor_from_c_str("/prefix/round_trip/direct_io_verify.txt")));
+    struct aws_byte_cursor object_path = aws_byte_cursor_from_buf(&path_buf);
+
+    struct aws_s3_tester_meta_request_options put_options = {
+        .allocator = allocator,
+        .meta_request_type = AWS_S3_META_REQUEST_TYPE_PUT_OBJECT,
+        .client = client,
+        .checksum_algorithm = AWS_SCA_CRC32,
+        .put_options =
+            {
+                .object_size_mb = 1,
+                .object_path_override = object_path,
+            },
+    };
+    ASSERT_SUCCESS(aws_s3_tester_send_meta_request_with_options(&tester, &put_options, NULL));
+
+    /* Download with O_DIRECT to a known file path */
+    const char *local_file_path = "/tmp/aws_s3_direct_io_crc32_verify";
+    struct aws_string *host_name =
+        aws_s3_tester_build_endpoint_string(allocator, &g_test_bucket_name, &g_test_s3_region);
+    struct aws_byte_cursor host_cursor = aws_byte_cursor_from_string(host_name);
+    struct aws_http_message *message = aws_s3_test_get_object_request_new(allocator, host_cursor, object_path);
+
+    struct aws_s3_checksum_config checksum_config = {
+        .validate_response_checksum = true,
+    };
+    struct aws_s3_file_io_options fio_opts = {
+        .direct_io = true,
+    };
+    struct aws_s3_meta_request_options meta_request_options = {
+        .type = AWS_S3_META_REQUEST_TYPE_GET_OBJECT,
+        .message = message,
+        .recv_filepath = aws_byte_cursor_from_c_str(local_file_path),
+        .checksum_config = &checksum_config,
+        .fio_opts = &fio_opts,
+    };
+
     struct aws_s3_meta_request_test_results meta_request_test_results;
     aws_s3_meta_request_test_results_init(&meta_request_test_results, allocator);
+    ASSERT_SUCCESS(aws_s3_tester_bind_meta_request(&tester, &meta_request_options, &meta_request_test_results));
+
+    struct aws_s3_meta_request *meta_request = aws_s3_client_make_meta_request(client, &meta_request_options);
+    ASSERT_NOT_NULL(meta_request);
+
+    aws_s3_tester_wait_for_meta_request_finish(&tester);
+    ASSERT_INT_EQUALS(AWS_ERROR_SUCCESS, meta_request_test_results.finished_error_code);
+    /* S3 response checksum was validated */
+    ASSERT_TRUE(meta_request_test_results.did_validate);
+    ASSERT_INT_EQUALS(AWS_SCA_CRC32, meta_request_test_results.validation_algorithm);
+
+    aws_s3_meta_request_release(meta_request);
+    aws_s3_tester_wait_for_meta_request_shutdown(&tester);
+
+    /* Read file back from disk and compute CRC32.
+     * The tester uploads content using AWS_AUTOGEN_LOREM_IPSUM pattern. */
+    size_t expected_size = MB_TO_BYTES(1);
+    struct aws_byte_buf expected_buf;
+    s_byte_buf_init_autogenned(&expected_buf, allocator, expected_size, AWS_AUTOGEN_LOREM_IPSUM);
+    uint32_t expected_crc = aws_checksums_crc32(expected_buf.buffer, (int)expected_buf.len, 0);
+
+    FILE *verify_file = aws_fopen(local_file_path, "rb");
+    ASSERT_NOT_NULL(verify_file);
+    struct aws_byte_buf file_buf;
+    aws_byte_buf_init(&file_buf, allocator, expected_size);
+    file_buf.len = fread(file_buf.buffer, 1, expected_size, verify_file);
+    fclose(verify_file);
+
+    ASSERT_UINT_EQUALS(expected_size, file_buf.len);
+    uint32_t actual_crc = aws_checksums_crc32(file_buf.buffer, (int)file_buf.len, 0);
+    ASSERT_UINT_EQUALS(expected_crc, actual_crc);
+
+    /* Cleanup */
+    remove(local_file_path);
+    aws_byte_buf_clean_up(&file_buf);
+    aws_byte_buf_clean_up(&expected_buf);
+    aws_s3_meta_request_test_results_clean_up(&meta_request_test_results);
+    aws_http_message_release(message);
+    aws_string_destroy(host_name);
+    aws_byte_buf_clean_up(&path_buf);
+    aws_s3_client_release(client);
+    aws_s3_tester_clean_up(&tester);
+    return 0;
+}
+
+AWS_TEST_CASE(
+    test_s3_get_object_file_path_direct_io_unsupported_append,
+    s_test_s3_get_object_file_path_direct_io_unsupported_append)
+static int s_test_s3_get_object_file_path_direct_io_unsupported_append(struct aws_allocator *allocator, void *ctx) {
+    (void)ctx;
+
+    struct aws_s3_tester tester;
+    ASSERT_SUCCESS(aws_s3_tester_init(allocator, &tester));
+
+    struct aws_s3_tester_client_options client_options = {
+        .part_size = MB_TO_BYTES(5),
+    };
+
     struct aws_s3_client *client = NULL;
     ASSERT_SUCCESS(aws_s3_tester_client_new(&tester, &client_options, &client));
 
@@ -1711,29 +1819,69 @@ static int s_test_s3_get_object_file_path_direct_io_to_position(struct aws_alloc
     };
 
     struct aws_byte_cursor object_path = aws_byte_cursor_from_c_str("/pre-existing-1MB");
-    uint64_t pre_exist_file_length = 10;
     struct aws_s3_tester_meta_request_options get_options = {
         .allocator = allocator,
         .meta_request_type = AWS_S3_META_REQUEST_TYPE_GET_OBJECT,
-        .validate_type = AWS_S3_TESTER_VALIDATE_TYPE_EXPECT_SUCCESS,
+        .validate_type = AWS_S3_TESTER_VALIDATE_TYPE_EXPECT_FAILURE,
+        .client = client,
+        .fio_opts = &fio_opts,
+        .get_options =
+            {
+                .object_path = object_path,
+                .file_on_disk = true,
+                .recv_file_option = AWS_S3_RECV_FILE_CREATE_OR_APPEND,
+            },
+    };
+
+    ASSERT_SUCCESS(aws_s3_tester_send_meta_request_with_options(&tester, &get_options, NULL));
+
+    aws_s3_client_release(client);
+    aws_s3_tester_clean_up(&tester);
+    return 0;
+}
+
+AWS_TEST_CASE(
+    test_s3_get_object_file_path_direct_io_unsupported_write_to_position,
+    s_test_s3_get_object_file_path_direct_io_unsupported_write_to_position)
+static int s_test_s3_get_object_file_path_direct_io_unsupported_write_to_position(
+    struct aws_allocator *allocator,
+    void *ctx) {
+    (void)ctx;
+
+    struct aws_s3_tester tester;
+    ASSERT_SUCCESS(aws_s3_tester_init(allocator, &tester));
+
+    struct aws_s3_tester_client_options client_options = {
+        .part_size = MB_TO_BYTES(5),
+    };
+
+    struct aws_s3_client *client = NULL;
+    ASSERT_SUCCESS(aws_s3_tester_client_new(&tester, &client_options, &client));
+
+    struct aws_s3_file_io_options fio_opts = {
+        .direct_io = true,
+    };
+
+    struct aws_byte_cursor object_path = aws_byte_cursor_from_c_str("/pre-existing-1MB");
+    struct aws_s3_tester_meta_request_options get_options = {
+        .allocator = allocator,
+        .meta_request_type = AWS_S3_META_REQUEST_TYPE_GET_OBJECT,
+        .validate_type = AWS_S3_TESTER_VALIDATE_TYPE_EXPECT_FAILURE,
         .client = client,
         .fio_opts = &fio_opts,
         .get_options =
             {
                 .object_path = object_path,
                 .file_on_disk = true,
                 .recv_file_option = AWS_S3_RECV_FILE_WRITE_TO_POSITION,
-                .recv_file_position = 20,
-                .pre_exist_file_length = pre_exist_file_length,
+                .recv_file_position = 4096,
+                .pre_exist_file_length = 10,
             },
     };
 
-    ASSERT_SUCCESS(aws_s3_tester_send_meta_request_with_options(&tester, &get_options, &meta_request_test_results));
-    ASSERT_UINT_EQUALS(
-        get_options.get_options.recv_file_position + MB_TO_BYTES(1), meta_request_test_results.received_file_size);
+    ASSERT_SUCCESS(aws_s3_tester_send_meta_request_with_options(&tester, &get_options, NULL));
 
-    aws_s3_meta_request_test_results_clean_up(&meta_request_test_results);
-    client = aws_s3_client_release(client);
+    aws_s3_client_release(client);
     aws_s3_tester_clean_up(&tester);
     return 0;
 }