[developer] Re: [openzfs/openzfs] 9091 Add mdb smart write (#544)
@sdimitro pushed 1 commit. a23de99 get rid of redundant check -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/openzfs/openzfs/pull/544/files/667119b38ecaf1f1a540975a8b63a7189ee9eaeb..a23de999d1a50e209c43bcdaadb8a29029c50e53 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Ta82fe96bb714cae3-M06d729e633b995fefc2486ad Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 9091 Add mdb smart write (#544)
sdimitro commented on this pull request.
> @@ -198,6 +204,100 @@ write_uint64(mdb_tgt_as_t as, mdb_tgt_addr_t addr,
> uint64_t n, uint_t rdback)
return (addr + sizeof (n));
}
+/*
+ * Writes to objects of size 1, 2, 4, or 8 bytes. The function
+ * doesn't care if the object is a number or not (e.g. it could
+ * be a byte array, or a struct) as long as the size of the write
+ * is one of the aforementioned ones.
+ */
+static mdb_tgt_addr_t
+write_var_uint(mdb_tgt_as_t as, mdb_tgt_addr_t addr, uint64_t val, size_t size,
+uint_t rdback)
+{
+ if (size > MDB_UINT_WRITE_MAXBYTES) {
I agree. Since that is pretty easy to test and we end up getting rid of the
extra constant I can re-iterate once more.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/544#discussion_r168258623
--
openzfs-developer
Archives:
https://openzfs.topicbox.com/groups/developer/discussions/Ta82fe96bb714cae3-M7d71c872e189a14a9a01d4fc
Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 9091 Add mdb smart write (#544)
rmustacc commented on this pull request.
> @@ -198,6 +204,100 @@ write_uint64(mdb_tgt_as_t as, mdb_tgt_addr_t addr,
> uint64_t n, uint_t rdback)
return (addr + sizeof (n));
}
+/*
+ * Writes to objects of size 1, 2, 4, or 8 bytes. The function
+ * doesn't care if the object is a number or not (e.g. it could
+ * be a byte array, or a struct) as long as the size of the write
+ * is one of the aforementioned ones.
+ */
+static mdb_tgt_addr_t
+write_var_uint(mdb_tgt_as_t as, mdb_tgt_addr_t addr, uint64_t val, size_t size,
+uint_t rdback)
+{
+ if (size > MDB_UINT_WRITE_MAXBYTES) {
Looking at this again, given that we are checking the size down below and
adjusting it based on a uint64_t do we actually need this block here? I just
realized given that we'll check the size in the switch statement, it may end up
being redundant. But, honestly, it doesn't matter either way.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/544#pullrequestreview-96594391
--
openzfs-developer
Archives:
https://openzfs.topicbox.com/groups/developer/discussions/Ta82fe96bb714cae3-M3440619a9871a4154a7bca7e
Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 9091 Add mdb smart write (#544)
Review updated. @rmustacc if there is no further feedback, do we have your approval for this review? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/544#issuecomment-365305966 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Ta82fe96bb714cae3-M17128fb97715f1bc391d3151 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 9091 Add mdb smart write (#544)
@sdimitro pushed 1 commit. a96b5da apply feedback -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/openzfs/openzfs/pull/544/files/f17764a59fa855e38e6044d0020648f07dd07599..a96b5daac5b0059e9074f0b59b8f9529d2157797 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/Ta82fe96bb714cae3-M48b5f4a0ef5423040df0fa4a Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 9091 Add mdb smart write (#544)
rmustacc commented on this pull request.
> @@ -198,6 +198,100 @@ write_uint64(mdb_tgt_as_t as, mdb_tgt_addr_t addr,
> uint64_t n, uint_t rdback)
return (addr + sizeof (n));
}
+/*
+ * Writes to objects of size 1, 2, 4, or 8 bytes. The function
+ * doesn't care if the object is a number or not (e.g. it could
+ * be a byte array, or a struct) as long as the size of the write
+ * is one of the aforementioned ones.
+ */
+static mdb_tgt_addr_t
+write_var_uint(mdb_tgt_as_t as, mdb_tgt_addr_t addr, uint64_t val, size_t size,
+uint_t rdback)
+{
+ if (size > sizeof (uintptr_t)) {
It might be, but even that is a little questionable. The max we have in the
code is 8 and we're not relating that to a type. So I'd probably actually use
that and just make a `#define` with a comment that explains its the max size
that the targets can write today. As it's really less specific to a type and
more specific to what the target allows.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/544#discussion_r167407506
--
openzfs-developer
Archives:
https://openzfs.topicbox.com/groups/developer/discussions/Ta82fe96bb714cae3-M47dbcd7b02256b2841b9b854
Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 9091 Add mdb smart write (#544)
sdimitro commented on this pull request.
> @@ -198,6 +198,100 @@ write_uint64(mdb_tgt_as_t as, mdb_tgt_addr_t addr,
> uint64_t n, uint_t rdback)
return (addr + sizeof (n));
}
+/*
+ * Writes to objects of size 1, 2, 4, or 8 bytes. The function
+ * doesn't care if the object is a number or not (e.g. it could
+ * be a byte array, or a struct) as long as the size of the write
+ * is one of the aforementioned ones.
+ */
+static mdb_tgt_addr_t
+write_var_uint(mdb_tgt_as_t as, mdb_tgt_addr_t addr, uint64_t val, size_t size,
+uint_t rdback)
+{
+ if (size > sizeof (uintptr_t)) {
You are right. The check here is to ensure that we don't write more bytes than
the longest integer supported.
Looking into the different integer types it seems like `uintmax_t` would be the
right type for this. Does this sound reasonable to you?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/544#discussion_r167407281
--
openzfs-developer
Archives:
https://openzfs.topicbox.com/groups/developer/discussions/Ta82fe96bb714cae3-Mf451df75631a9a9e19aafcad
Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 9091 Add mdb smart write (#544)
rmustacc commented on this pull request.
In general, this looks good. Thanks.
> @@ -198,6 +198,100 @@ write_uint64(mdb_tgt_as_t as, mdb_tgt_addr_t addr,
> uint64_t n, uint_t rdback)
return (addr + sizeof (n));
}
+/*
+ * Writes to objects of size 1, 2, 4, or 8 bytes. The function
+ * doesn't care if the object is a number or not (e.g. it could
+ * be a byte array, or a struct) as long as the size of the write
+ * is one of the aforementioned ones.
+ */
+static mdb_tgt_addr_t
+write_var_uint(mdb_tgt_as_t as, mdb_tgt_addr_t addr, uint64_t val, size_t size,
+uint_t rdback)
+{
+ if (size > sizeof (uintptr_t)) {
Why are we using the uintptr_t as the sentinel for the size? For example, if we
were building a 32-bit version of this, then we'd have an ILP32 system so the
uintptr_t would be 4 bytes; however, you can write 8 bytes.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/544#pullrequestreview-95628953
--
openzfs-developer
Archives:
https://openzfs.topicbox.com/groups/developer/discussions/Ta82fe96bb714cae3-M39dc308abc7dc1f1a20217a8
Powered by Topicbox: https://topicbox.com
