From 02fd7aef55c28e0f0797476ce7d26a6897ef0846 Mon Sep 17 00:00:00 2001
From: Ryan Tan <hahadaxigua@gmail.com>
Date: Sat, 27 Jul 2024 17:07:51 +0800
Subject: [PATCH 1/6] feat: separate part-size for PUT & GET

Signed-off-by: Ryan Tan <hahadaxigua@gmail.com>
---
 mountpoint-s3-client/src/failure_client.rs    |  8 ++-
 mountpoint-s3-client/src/mock_client.rs       |  6 +-
 .../src/mock_client/throughput_client.rs      |  8 ++-
 mountpoint-s3-client/src/object_client.rs     |  8 ++-
 mountpoint-s3-client/src/s3_crt_client.rs     | 58 +++++++++++++------
 .../src/s3_crt_client/get_object.rs           | 10 +++-
 .../src/s3_crt_client/put_object.rs           |  1 +
 mountpoint-s3-client/tests/get_object.rs      |  2 +-
 mountpoint-s3-client/tests/put_object.rs      |  2 +-
 mountpoint-s3/src/cli.rs                      | 30 +++++++++-
 mountpoint-s3/src/prefetch/part_stream.rs     |  2 +-
 mountpoint-s3/src/upload.rs                   |  2 +-
 12 files changed, 105 insertions(+), 32 deletions(-)

diff --git a/mountpoint-s3-client/src/failure_client.rs b/mountpoint-s3-client/src/failure_client.rs
index 983cb4eb1..e0f2b43e7 100644
--- a/mountpoint-s3-client/src/failure_client.rs
+++ b/mountpoint-s3-client/src/failure_client.rs
@@ -73,8 +73,12 @@ where
     type PutObjectRequest = FailurePutObjectRequest<Client, GetWrapperState>;
     type ClientError = Client::ClientError;
 
-    fn part_size(&self) -> Option<usize> {
-        self.client.part_size()
+    fn read_part_size(&self) -> Option<usize> {
+        self.client.read_part_size()
+    }
+
+    fn write_part_size(&self) -> Option<usize> {
+        self.client.write_part_size()
     }
 
     async fn delete_object(
diff --git a/mountpoint-s3-client/src/mock_client.rs b/mountpoint-s3-client/src/mock_client.rs
index dd170bf12..902b27c96 100644
--- a/mountpoint-s3-client/src/mock_client.rs
+++ b/mountpoint-s3-client/src/mock_client.rs
@@ -554,7 +554,11 @@ impl ObjectClient for MockClient {
     type PutObjectRequest = MockPutObjectRequest;
     type ClientError = MockClientError;
 
-    fn part_size(&self) -> Option<usize> {
+    fn read_part_size(&self) -> Option<usize> {
+        Some(self.config.part_size)
+    }
+
+    fn write_part_size(&self) -> Option<usize> {
         Some(self.config.part_size)
     }
 
diff --git a/mountpoint-s3-client/src/mock_client/throughput_client.rs b/mountpoint-s3-client/src/mock_client/throughput_client.rs
index 932829a4f..e3089ced4 100644
--- a/mountpoint-s3-client/src/mock_client/throughput_client.rs
+++ b/mountpoint-s3-client/src/mock_client/throughput_client.rs
@@ -97,8 +97,12 @@ impl ObjectClient for ThroughputMockClient {
     type PutObjectRequest = MockPutObjectRequest;
     type ClientError = MockClientError;
 
-    fn part_size(&self) -> Option<usize> {
-        self.inner.part_size()
+    fn read_part_size(&self) -> Option<usize> {
+        self.inner.read_part_size()
+    }
+
+    fn write_part_size(&self) -> Option<usize> {
+        self.inner.write_part_size()
     }
 
     async fn delete_object(
diff --git a/mountpoint-s3-client/src/object_client.rs b/mountpoint-s3-client/src/object_client.rs
index db8eba845..741c842d5 100644
--- a/mountpoint-s3-client/src/object_client.rs
+++ b/mountpoint-s3-client/src/object_client.rs
@@ -77,9 +77,13 @@ pub trait ObjectClient {
     type PutObjectRequest: PutObjectRequest<ClientError = Self::ClientError>;
     type ClientError: std::error::Error + ProvideErrorMetadata + Send + Sync + 'static;
 
-    /// Query the part size this client uses for PUT and GET operations to the object store. This
+    /// Query the part size this client uses for GET operations to the object store. This
     /// can be `None` if the client does not do multi-part operations.
-    fn part_size(&self) -> Option<usize>;
+    fn read_part_size(&self) -> Option<usize>;
+
+    /// Query the part size this client uses for PUT operations to the object store. This
+    /// can be `None` if the client does not do multi-part operations.
+    fn write_part_size(&self) -> Option<usize>;
 
     /// Delete a single object from the object store.
     ///
diff --git a/mountpoint-s3-client/src/s3_crt_client.rs b/mountpoint-s3-client/src/s3_crt_client.rs
index aa310a3ba..8bb8f783d 100644
--- a/mountpoint-s3-client/src/s3_crt_client.rs
+++ b/mountpoint-s3-client/src/s3_crt_client.rs
@@ -85,7 +85,8 @@ macro_rules! event {
 pub struct S3ClientConfig {
     auth_config: S3ClientAuthConfig,
     throughput_target_gbps: f64,
-    part_size: usize,
+    read_part_size: usize,
+    write_part_size: usize,
     endpoint_config: EndpointConfig,
     user_agent: Option<UserAgent>,
     request_payer: Option<String>,
@@ -101,7 +102,8 @@ impl Default for S3ClientConfig {
         Self {
             auth_config: Default::default(),
             throughput_target_gbps: 10.0,
-            part_size: DEFAULT_PART_SIZE,
+            read_part_size: DEFAULT_PART_SIZE,
+            write_part_size: DEFAULT_PART_SIZE,
             endpoint_config: EndpointConfig::new("us-east-1"),
             user_agent: None,
             request_payer: None,
@@ -128,7 +130,22 @@ impl S3ClientConfig {
     /// Set the part size for multi-part operations to S3 (both PUT and GET)
     #[must_use = "S3ClientConfig follows a builder pattern"]
     pub fn part_size(mut self, part_size: usize) -> Self {
-        self.part_size = part_size;
+        self.read_part_size = part_size;
+        self.write_part_size = part_size;
+        self
+    }
+
+    /// Set the part size for multi-part-get operations to S3.
+    #[must_use = "S3ClientConfig follows a builder pattern"]
+    pub fn read_part_size(mut self, size: usize) -> Self {
+        self.read_part_size = size;
+        self
+    }
+
+    /// Set the part size for multi-part-put operations to S3.
+    #[must_use = "S3ClientConfig follows a builder pattern"]
+    pub fn write_part_size(mut self, size: usize) -> Self {
+        self.write_part_size = size;
         self
     }
 
@@ -248,7 +265,8 @@ struct S3CrtClientInner {
     /// Here it will add the user agent prefix and s3 client information.
     user_agent_header: String,
     request_payer: Option<String>,
-    part_size: usize,
+    read_part_size: usize,
+    write_part_size: usize,
     bucket_owner: Option<String>,
     credentials_provider: Option<CredentialsProvider>,
     host_resolver: HostResolver,
@@ -352,13 +370,14 @@ impl S3CrtClientInner {
 
         // max_part_size is 5GB or less depending on the platform (4GB on 32-bit)
         let max_part_size = cmp::min(5_u64 * 1024 * 1024 * 1024, usize::MAX as u64) as usize;
-        if !(5 * 1024 * 1024..=max_part_size).contains(&config.part_size) {
-            return Err(NewClientError::InvalidConfiguration(format!(
-                "part size must be at between 5MiB and {}GiB",
-                max_part_size / 1024 / 1024 / 1024
-            )));
+        for part_size in [config.read_part_size, config.write_part_size] {
+            if !(5 * 1024 * 1024..=max_part_size).contains(&part_size) {
+                return Err(NewClientError::InvalidConfiguration(format!(
+                    "part size must be at between 5MiB and {}GiB",
+                    max_part_size / 1024 / 1024 / 1024
+                )));
+            }
         }
-        client_config.part_size(config.part_size);
 
         let user_agent = config.user_agent.unwrap_or_else(|| UserAgent::new(None));
         let user_agent_header = user_agent.build();
@@ -373,7 +392,8 @@ impl S3CrtClientInner {
             next_request_counter: AtomicU64::new(0),
             user_agent_header,
             request_payer: config.request_payer,
-            part_size: config.part_size,
+            read_part_size: config.read_part_size,
+            write_part_size: config.write_part_size,
             bucket_owner: config.bucket_owner,
             credentials_provider: Some(credentials_provider),
             host_resolver,
@@ -1145,11 +1165,18 @@ impl ObjectClient for S3CrtClient {
     type PutObjectRequest = S3PutObjectRequest;
     type ClientError = S3RequestError;
 
-    fn part_size(&self) -> Option<usize> {
+    fn read_part_size(&self) -> Option<usize> {
         // TODO: the CRT does some clamping to a max size rather than just swallowing the part size
         // we configured it with, so this might be wrong. Right now the only clamping is to the max
         // S3 part size (5GiB), so this shouldn't affect the result.
-        Some(self.inner.part_size)
+        Some(self.inner.read_part_size)
+    }
+
+    fn write_part_size(&self) -> Option<usize> {
+        // TODO: the CRT does some clamping to a max size rather than just swallowing the part size
+        // we configured it with, so this might be wrong. Right now the only clamping is to the max
+        // S3 part size (5GiB), so this shouldn't affect the result.
+        Some(self.inner.write_part_size)
     }
 
     async fn delete_object(
@@ -1225,10 +1252,7 @@ mod tests {
 
     /// Test explicit validation in [Client::new]
     fn client_new_fails_with_invalid_part_size(part_size: usize) {
-        let config = S3ClientConfig {
-            part_size,
-            ..Default::default()
-        };
+        let config = S3ClientConfig::default().part_size(part_size);
         let e = S3CrtClient::new(config).expect_err("creating a new client should fail");
         let message = if cfg!(target_pointer_width = "64") {
             "invalid configuration: part size must be at between 5MiB and 5GiB".to_string()
diff --git a/mountpoint-s3-client/src/s3_crt_client/get_object.rs b/mountpoint-s3-client/src/s3_crt_client/get_object.rs
index 847ba5156..9137c5a4a 100644
--- a/mountpoint-s3-client/src/s3_crt_client/get_object.rs
+++ b/mountpoint-s3-client/src/s3_crt_client/get_object.rs
@@ -15,6 +15,8 @@ use pin_project::pin_project;
 use crate::object_client::{ETag, GetBodyPart, GetObjectError, ObjectClientError, ObjectClientResult};
 use crate::s3_crt_client::{GetObjectRequest, S3CrtClient, S3HttpRequest, S3Operation, S3RequestError};
 
+use super::S3CrtClientInner;
+
 impl S3CrtClient {
     /// Create and begin a new GetObject request. The returned [GetObjectRequest] is a [Stream] of
     /// body parts of the object, which will be delivered in order.
@@ -59,10 +61,12 @@ impl S3CrtClient {
 
         let (sender, receiver) = futures::channel::mpsc::unbounded();
 
-        let request = self.inner.make_meta_request(
-            message,
-            S3Operation::GetObject,
+        let mut options = S3CrtClientInner::new_meta_request_options(message, S3Operation::GetObject);
+        options.part_size(self.inner.write_part_size as u64);
+        let request = self.inner.make_meta_request_from_options(
+            options,
             span,
+            |_| (),
             |_, _| (),
             move |offset, data| {
                 let _ = sender.unbounded_send(Ok((offset, data.into())));
diff --git a/mountpoint-s3-client/src/s3_crt_client/put_object.rs b/mountpoint-s3-client/src/s3_crt_client/put_object.rs
index 69d6137c4..cf0538042 100644
--- a/mountpoint-s3-client/src/s3_crt_client/put_object.rs
+++ b/mountpoint-s3-client/src/s3_crt_client/put_object.rs
@@ -69,6 +69,7 @@ impl S3CrtClient {
         let mut options = S3CrtClientInner::new_meta_request_options(message, S3Operation::PutObject);
         options.send_using_async_writes(true);
         options.on_upload_review(move |review| callback.invoke(review));
+        options.part_size(self.inner.write_part_size as u64);
 
         // Before the first write, we need to await for the multi-part upload to be created, so we can report errors.
         // To do so, we need to detect one of two events (whichever comes first):
diff --git a/mountpoint-s3-client/tests/get_object.rs b/mountpoint-s3-client/tests/get_object.rs
index 10e9fe309..916bfaf2a 100644
--- a/mountpoint-s3-client/tests/get_object.rs
+++ b/mountpoint-s3-client/tests/get_object.rs
@@ -97,7 +97,7 @@ async fn test_get_object_backpressure(size: usize, range: Option<Range<u64>>) {
 async fn verify_backpressure_get_object() {
     let initial_window_size = 256;
     let client: S3CrtClient = get_test_backpressure_client(initial_window_size);
-    let part_size = client.part_size().unwrap();
+    let part_size = client.read_part_size().unwrap();
 
     let size = part_size * 2;
     let range = 0..(part_size + 1) as u64;
diff --git a/mountpoint-s3-client/tests/put_object.rs b/mountpoint-s3-client/tests/put_object.rs
index 084ced75e..5c24e881b 100644
--- a/mountpoint-s3-client/tests/put_object.rs
+++ b/mountpoint-s3-client/tests/put_object.rs
@@ -217,7 +217,7 @@ async fn test_put_object_write_cancelled() {
         .expect("put_object should succeed");
 
     // Write a multiple of `part_size` to ensure it will not complete immediately.
-    let full_size = client.part_size().unwrap() * 10;
+    let full_size = client.write_part_size().unwrap() * 10;
     let buffer = vec![0u8; full_size];
 
     // Complete one write to ensure the MPU was created and the buffer for the upload request is available.
diff --git a/mountpoint-s3/src/cli.rs b/mountpoint-s3/src/cli.rs
index 7fdb18b8f..d02702c8f 100644
--- a/mountpoint-s3/src/cli.rs
+++ b/mountpoint-s3/src/cli.rs
@@ -160,11 +160,38 @@ pub struct CliArgs {
         long,
         help = "Part size for multi-part GET and PUT",
         default_value = "8388608",
+        value_name = "Size",
         value_parser = value_parser!(u64).range(1..usize::MAX as u64),
         help_heading = CLIENT_OPTIONS_HEADER
     )]
     pub part_size: u64,
 
+    #[clap(
+        long,
+        long_help = "Optional, part size for multi-part GET operation. (in Bytes)
+
+Tweak this argument for better download throughput and efficiency.
+
+Original part-size will be used if not specified.",
+        value_name = "Size",
+        value_parser = value_parser!(u64).range(1..usize::MAX as u64),
+        help_heading = CLIENT_OPTIONS_HEADER
+    )]
+    pub read_part_size: Option<u64>,
+
+    #[clap(
+        long,
+        long_help = "Optional, part size for multi-part PUT operations. (in Bytes)
+
+Tweaking this argument for better upload speed and efficiency.
+
+Original part-size will be used if not specified.",
+        value_name = "Size",
+        value_parser = value_parser!(u64).range(1..usize::MAX as u64),
+        help_heading = CLIENT_OPTIONS_HEADER
+    )]
+    pub write_part_size: Option<u64>,
+
     #[clap(
         long,
         help = "Owner UID [default: current user's UID]",
@@ -618,7 +645,8 @@ pub fn create_s3_client(args: &CliArgs) -> anyhow::Result<(S3CrtClient, EventLoo
     let mut client_config = S3ClientConfig::new()
         .auth_config(auth_config)
         .throughput_target_gbps(throughput_target_gbps)
-        .part_size(args.part_size as usize)
+        .read_part_size(args.read_part_size.unwrap_or(args.part_size) as usize)
+        .write_part_size(args.write_part_size.unwrap_or(args.part_size) as usize)
         .user_agent(user_agent);
     if args.requester_pays {
         client_config = client_config.request_payer("requester");
diff --git a/mountpoint-s3/src/prefetch/part_stream.rs b/mountpoint-s3/src/prefetch/part_stream.rs
index 43dcdd0d4..52f759e63 100644
--- a/mountpoint-s3/src/prefetch/part_stream.rs
+++ b/mountpoint-s3/src/prefetch/part_stream.rs
@@ -176,7 +176,7 @@ where
         Client: ObjectClient + Clone + Send + Sync + 'static,
     {
         assert!(preferred_part_size > 0);
-        let request_range = range.align(client.part_size().unwrap_or(8 * 1024 * 1024) as u64, true);
+        let request_range = range.align(client.read_part_size().unwrap_or(8 * 1024 * 1024) as u64, true);
         let start = request_range.start();
         let size = request_range.len();
 
diff --git a/mountpoint-s3/src/upload.rs b/mountpoint-s3/src/upload.rs
index 83f6f0b34..da4039846 100644
--- a/mountpoint-s3/src/upload.rs
+++ b/mountpoint-s3/src/upload.rs
@@ -126,7 +126,7 @@ impl<Client: ObjectClient> UploadRequest<Client> {
         let request = inner.client.put_object(bucket, key, &params).await?;
         let maximum_upload_size = inner
             .client
-            .part_size()
+            .write_part_size()
             .map(|ps| ps.saturating_mul(MAX_S3_MULTIPART_UPLOAD_PARTS));
 
         Ok(Self {

From e5d60c8087b976041893c50db04d4181fbc41564 Mon Sep 17 00:00:00 2001
From: Ryan Tan <hahadaxigua@gmail.com>
Date: Sat, 27 Jul 2024 17:13:42 +0800
Subject: [PATCH 2/6] chore: follow import style

Signed-off-by: Ryan Tan <hahadaxigua@gmail.com>
---
 mountpoint-s3-client/src/s3_crt_client/get_object.rs | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mountpoint-s3-client/src/s3_crt_client/get_object.rs b/mountpoint-s3-client/src/s3_crt_client/get_object.rs
index 9137c5a4a..7f9b349d8 100644
--- a/mountpoint-s3-client/src/s3_crt_client/get_object.rs
+++ b/mountpoint-s3-client/src/s3_crt_client/get_object.rs
@@ -13,9 +13,9 @@ use mountpoint_s3_crt::s3::client::MetaRequestResult;
 use pin_project::pin_project;
 
 use crate::object_client::{ETag, GetBodyPart, GetObjectError, ObjectClientError, ObjectClientResult};
-use crate::s3_crt_client::{GetObjectRequest, S3CrtClient, S3HttpRequest, S3Operation, S3RequestError};
-
-use super::S3CrtClientInner;
+use crate::s3_crt_client::{
+    GetObjectRequest, S3CrtClient, S3CrtClientInner, S3HttpRequest, S3Operation, S3RequestError,
+};
 
 impl S3CrtClient {
     /// Create and begin a new GetObject request. The returned [GetObjectRequest] is a [Stream] of

From 569b5359cef5207f12d44b21344a8b113ffd0599 Mon Sep 17 00:00:00 2001
From: Ryan Tan <hahadaxigua@gmail.com>
Date: Mon, 29 Jul 2024 22:49:00 +0800
Subject: [PATCH 3/6] fix: simplify cli help; make separated part-size conflict
 with old one; use read_part_size when get

Signed-off-by: Ryan Tan <hahadaxigua@gmail.com>
---
 mountpoint-s3-client/src/s3_crt_client.rs     | 14 +++---
 .../src/s3_crt_client/get_object.rs           |  2 +-
 mountpoint-s3/src/bin/mock-mount-s3.rs        |  2 +-
 mountpoint-s3/src/cli.rs                      | 43 +++++++++++--------
 4 files changed, 33 insertions(+), 28 deletions(-)

diff --git a/mountpoint-s3-client/src/s3_crt_client.rs b/mountpoint-s3-client/src/s3_crt_client.rs
index 8bb8f783d..c1e24684e 100644
--- a/mountpoint-s3-client/src/s3_crt_client.rs
+++ b/mountpoint-s3-client/src/s3_crt_client.rs
@@ -98,24 +98,25 @@ pub struct S3ClientConfig {
 
 impl Default for S3ClientConfig {
     fn default() -> Self {
-        const DEFAULT_PART_SIZE: usize = 8 * 1024 * 1024;
         Self {
             auth_config: Default::default(),
             throughput_target_gbps: 10.0,
-            read_part_size: DEFAULT_PART_SIZE,
-            write_part_size: DEFAULT_PART_SIZE,
+            read_part_size: Self::DEFAULT_PART_SIZE as usize,
+            write_part_size: Self::DEFAULT_PART_SIZE as usize,
             endpoint_config: EndpointConfig::new("us-east-1"),
             user_agent: None,
             request_payer: None,
             bucket_owner: None,
             max_attempts: None,
             read_backpressure: false,
-            initial_read_window: DEFAULT_PART_SIZE,
+            initial_read_window: Self::DEFAULT_PART_SIZE as usize,
         }
     }
 }
 
 impl S3ClientConfig {
+    pub const DEFAULT_PART_SIZE: u64 = 8 * 1024 * 1024;
+
     pub fn new() -> Self {
         Self::default()
     }
@@ -370,6 +371,7 @@ impl S3CrtClientInner {
 
         // max_part_size is 5GB or less depending on the platform (4GB on 32-bit)
         let max_part_size = cmp::min(5_u64 * 1024 * 1024 * 1024, usize::MAX as u64) as usize;
+        // TODO: Review the part size validation for read_part_size, read_part_size can have a more relax limit.
         for part_size in [config.read_part_size, config.write_part_size] {
             if !(5 * 1024 * 1024..=max_part_size).contains(&part_size) {
                 return Err(NewClientError::InvalidConfiguration(format!(
@@ -1166,9 +1168,6 @@ impl ObjectClient for S3CrtClient {
     type ClientError = S3RequestError;
 
     fn read_part_size(&self) -> Option<usize> {
-        // TODO: the CRT does some clamping to a max size rather than just swallowing the part size
-        // we configured it with, so this might be wrong. Right now the only clamping is to the max
-        // S3 part size (5GiB), so this shouldn't affect the result.
         Some(self.inner.read_part_size)
     }
 
@@ -1176,6 +1175,7 @@ impl ObjectClient for S3CrtClient {
         // TODO: the CRT does some clamping to a max size rather than just swallowing the part size
         // we configured it with, so this might be wrong. Right now the only clamping is to the max
         // S3 part size (5GiB), so this shouldn't affect the result.
+        // https://github.com/awslabs/aws-c-s3/blob/94e3342c12833c5199/source/s3_client.c#L337-L344
         Some(self.inner.write_part_size)
     }
 
diff --git a/mountpoint-s3-client/src/s3_crt_client/get_object.rs b/mountpoint-s3-client/src/s3_crt_client/get_object.rs
index 7f9b349d8..4fe31eeb1 100644
--- a/mountpoint-s3-client/src/s3_crt_client/get_object.rs
+++ b/mountpoint-s3-client/src/s3_crt_client/get_object.rs
@@ -62,7 +62,7 @@ impl S3CrtClient {
         let (sender, receiver) = futures::channel::mpsc::unbounded();
 
         let mut options = S3CrtClientInner::new_meta_request_options(message, S3Operation::GetObject);
-        options.part_size(self.inner.write_part_size as u64);
+        options.part_size(self.inner.read_part_size as u64);
         let request = self.inner.make_meta_request_from_options(
             options,
             span,
diff --git a/mountpoint-s3/src/bin/mock-mount-s3.rs b/mountpoint-s3/src/bin/mock-mount-s3.rs
index 2ed54debf..fc6fb83b1 100644
--- a/mountpoint-s3/src/bin/mock-mount-s3.rs
+++ b/mountpoint-s3/src/bin/mock-mount-s3.rs
@@ -37,7 +37,7 @@ fn create_mock_client(args: &CliArgs) -> anyhow::Result<(ThroughputMockClient, T
 
     let config = MockClientConfig {
         bucket: args.bucket_name.clone(),
-        part_size: args.part_size as usize,
+        part_size: args.part_size.unwrap() as usize,
         unordered_list_seed: None,
         ..Default::default()
     };
diff --git a/mountpoint-s3/src/cli.rs b/mountpoint-s3/src/cli.rs
index d02702c8f..6c7ec947e 100644
--- a/mountpoint-s3/src/cli.rs
+++ b/mountpoint-s3/src/cli.rs
@@ -158,37 +158,31 @@ pub struct CliArgs {
 
     #[clap(
         long,
-        help = "Part size for multi-part GET and PUT",
+        help = "Part size for multi-part GET and PUT in bytes",
         default_value = "8388608",
-        value_name = "Size",
+        value_name = "SIZE",
         value_parser = value_parser!(u64).range(1..usize::MAX as u64),
         help_heading = CLIENT_OPTIONS_HEADER
     )]
-    pub part_size: u64,
+    pub part_size: Option<u64>,
 
     #[clap(
         long,
-        long_help = "Optional, part size for multi-part GET operation. (in Bytes)
-
-Tweak this argument for better download throughput and efficiency.
-
-Original part-size will be used if not specified.",
-        value_name = "Size",
+        help = "Part size for multi-part GET in bytes [default: 8388608]",
+        value_name = "SIZE",
         value_parser = value_parser!(u64).range(1..usize::MAX as u64),
-        help_heading = CLIENT_OPTIONS_HEADER
+        help_heading = CLIENT_OPTIONS_HEADER,
+        conflicts_with = "part_size",
     )]
     pub read_part_size: Option<u64>,
 
     #[clap(
         long,
-        long_help = "Optional, part size for multi-part PUT operations. (in Bytes)
-
-Tweaking this argument for better upload speed and efficiency.
-
-Original part-size will be used if not specified.",
-        value_name = "Size",
+        help = "Part size for multi-part PUT in bytes [default: 8388608]",
+        value_name = "SIZE",
         value_parser = value_parser!(u64).range(1..usize::MAX as u64),
-        help_heading = CLIENT_OPTIONS_HEADER
+        help_heading = CLIENT_OPTIONS_HEADER,
+        conflicts_with = "part_size",
     )]
     pub write_part_size: Option<u64>,
 
@@ -642,11 +636,22 @@ pub fn create_s3_client(args: &CliArgs) -> anyhow::Result<(S3CrtClient, EventLoo
         user_agent.key_value("mp-cache-ttl", &ttl.to_string());
     }
 
+    let (read_part_size, write_part_size) = match (args.read_part_size, args.write_part_size) {
+        (None, None) => {
+            let size = args.part_size.unwrap_or(S3ClientConfig::DEFAULT_PART_SIZE);
+            (size, size)
+        }
+        _ => (
+            args.read_part_size.unwrap_or(S3ClientConfig::DEFAULT_PART_SIZE),
+            args.write_part_size.unwrap_or(S3ClientConfig::DEFAULT_PART_SIZE),
+        ),
+    };
+
     let mut client_config = S3ClientConfig::new()
         .auth_config(auth_config)
         .throughput_target_gbps(throughput_target_gbps)
-        .read_part_size(args.read_part_size.unwrap_or(args.part_size) as usize)
-        .write_part_size(args.write_part_size.unwrap_or(args.part_size) as usize)
+        .read_part_size(read_part_size as usize)
+        .write_part_size(write_part_size as usize)
         .user_agent(user_agent);
     if args.requester_pays {
         client_config = client_config.request_payer("requester");

From b362d4334282f9a3ac399c859f57a6c637c3290d Mon Sep 17 00:00:00 2001
From: Ryan Tan <hahadaxigua@gmail.com>
Date: Mon, 29 Jul 2024 23:10:05 +0800
Subject: [PATCH 4/6] Verify new separated part size arg is conflicted with old
 one

Signed-off-by: Ryan Tan <hahadaxigua@gmail.com>
---
 mountpoint-s3/tests/cli.rs | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/mountpoint-s3/tests/cli.rs b/mountpoint-s3/tests/cli.rs
index 7975505d1..3ce5093ed 100644
--- a/mountpoint-s3/tests/cli.rs
+++ b/mountpoint-s3/tests/cli.rs
@@ -277,3 +277,27 @@ fn sse_key_not_allowed_with_aes256() -> Result<(), Box<dyn std::error::Error>> {
 
     Ok(())
 }
+
+#[test_case(Some(1024), Some(1024))]
+#[test_case(None, Some(1024))]
+#[test_case(Some(1024), None)]
+fn verify_new_part_size_config_conflict_with_old_one(
+    read_part_size: Option<u64>,
+    write_part_size: Option<u64>,
+) -> Result<(), Box<dyn std::error::Error>> {
+    let dir = assert_fs::TempDir::new()?;
+    let mut cmd = Command::cargo_bin("mount-s3")?;
+    cmd.arg("test-bucket").arg(dir.path()).arg("--part-size=1024");
+
+    if let Some(read_part_size) = read_part_size {
+        cmd.arg(format!("--read-part-size={}", read_part_size));
+    }
+    if let Some(write_part_size) = write_part_size {
+        cmd.arg(format!("--write-part-size={}", write_part_size));
+    }
+
+    let error_message = "cannot be used with";
+    cmd.assert().failure().stderr(predicate::str::contains(error_message));
+
+    Ok(())
+}

From 63163b7fc628fd6aba26fcc8e719708084d40dc7 Mon Sep 17 00:00:00 2001
From: Ryan Tan <hahadaxigua@gmail.com>
Date: Mon, 29 Jul 2024 23:15:41 +0800
Subject: [PATCH 5/6] Drop Option<u64> on part-size

Signed-off-by: Ryan Tan <hahadaxigua@gmail.com>
---
 mountpoint-s3-client/src/s3_crt_client.rs |  9 ++++-----
 mountpoint-s3/src/bin/mock-mount-s3.rs    |  2 +-
 mountpoint-s3/src/cli.rs                  | 11 ++++-------
 3 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/mountpoint-s3-client/src/s3_crt_client.rs b/mountpoint-s3-client/src/s3_crt_client.rs
index c1e24684e..25747ef96 100644
--- a/mountpoint-s3-client/src/s3_crt_client.rs
+++ b/mountpoint-s3-client/src/s3_crt_client.rs
@@ -98,25 +98,24 @@ pub struct S3ClientConfig {
 
 impl Default for S3ClientConfig {
     fn default() -> Self {
+        const DEFAULT_PART_SIZE: usize = 8 * 1024 * 1024;
         Self {
             auth_config: Default::default(),
             throughput_target_gbps: 10.0,
-            read_part_size: Self::DEFAULT_PART_SIZE as usize,
-            write_part_size: Self::DEFAULT_PART_SIZE as usize,
+            read_part_size: DEFAULT_PART_SIZE,
+            write_part_size: DEFAULT_PART_SIZE,
             endpoint_config: EndpointConfig::new("us-east-1"),
             user_agent: None,
             request_payer: None,
             bucket_owner: None,
             max_attempts: None,
             read_backpressure: false,
-            initial_read_window: Self::DEFAULT_PART_SIZE as usize,
+            initial_read_window: DEFAULT_PART_SIZE,
         }
     }
 }
 
 impl S3ClientConfig {
-    pub const DEFAULT_PART_SIZE: u64 = 8 * 1024 * 1024;
-
     pub fn new() -> Self {
         Self::default()
     }
diff --git a/mountpoint-s3/src/bin/mock-mount-s3.rs b/mountpoint-s3/src/bin/mock-mount-s3.rs
index fc6fb83b1..2ed54debf 100644
--- a/mountpoint-s3/src/bin/mock-mount-s3.rs
+++ b/mountpoint-s3/src/bin/mock-mount-s3.rs
@@ -37,7 +37,7 @@ fn create_mock_client(args: &CliArgs) -> anyhow::Result<(ThroughputMockClient, T
 
     let config = MockClientConfig {
         bucket: args.bucket_name.clone(),
-        part_size: args.part_size.unwrap() as usize,
+        part_size: args.part_size as usize,
         unordered_list_seed: None,
         ..Default::default()
     };
diff --git a/mountpoint-s3/src/cli.rs b/mountpoint-s3/src/cli.rs
index 6c7ec947e..450e78164 100644
--- a/mountpoint-s3/src/cli.rs
+++ b/mountpoint-s3/src/cli.rs
@@ -164,7 +164,7 @@ pub struct CliArgs {
         value_parser = value_parser!(u64).range(1..usize::MAX as u64),
         help_heading = CLIENT_OPTIONS_HEADER
     )]
-    pub part_size: Option<u64>,
+    pub part_size: u64,
 
     #[clap(
         long,
@@ -637,13 +637,10 @@ pub fn create_s3_client(args: &CliArgs) -> anyhow::Result<(S3CrtClient, EventLoo
     }
 
     let (read_part_size, write_part_size) = match (args.read_part_size, args.write_part_size) {
-        (None, None) => {
-            let size = args.part_size.unwrap_or(S3ClientConfig::DEFAULT_PART_SIZE);
-            (size, size)
-        }
+        (None, None) => (args.part_size, args.part_size),
         _ => (
-            args.read_part_size.unwrap_or(S3ClientConfig::DEFAULT_PART_SIZE),
-            args.write_part_size.unwrap_or(S3ClientConfig::DEFAULT_PART_SIZE),
+            args.read_part_size.unwrap_or(args.part_size),
+            args.write_part_size.unwrap_or(args.part_size),
         ),
     };
 

From 4a9f7832cf0fc1e2d48c9b3b0ba65ca2c4a72aba Mon Sep 17 00:00:00 2001
From: Ryan Tan <hahadaxigua@gmail.com>
Date: Tue, 30 Jul 2024 00:34:09 +0800
Subject: [PATCH 6/6] Move part-size back

Signed-off-by: Ryan Tan <hahadaxigua@gmail.com>
---
 mountpoint-s3/src/cli.rs | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/mountpoint-s3/src/cli.rs b/mountpoint-s3/src/cli.rs
index 450e78164..7f06959a6 100644
--- a/mountpoint-s3/src/cli.rs
+++ b/mountpoint-s3/src/cli.rs
@@ -636,19 +636,11 @@ pub fn create_s3_client(args: &CliArgs) -> anyhow::Result<(S3CrtClient, EventLoo
         user_agent.key_value("mp-cache-ttl", &ttl.to_string());
     }
 
-    let (read_part_size, write_part_size) = match (args.read_part_size, args.write_part_size) {
-        (None, None) => (args.part_size, args.part_size),
-        _ => (
-            args.read_part_size.unwrap_or(args.part_size),
-            args.write_part_size.unwrap_or(args.part_size),
-        ),
-    };
-
     let mut client_config = S3ClientConfig::new()
         .auth_config(auth_config)
         .throughput_target_gbps(throughput_target_gbps)
-        .read_part_size(read_part_size as usize)
-        .write_part_size(write_part_size as usize)
+        .read_part_size(args.read_part_size.unwrap_or(args.part_size) as usize)
+        .write_part_size(args.write_part_size.unwrap_or(args.part_size) as usize)
         .user_agent(user_agent);
     if args.requester_pays {
         client_config = client_config.request_payer("requester");