[PATCH 3/5] zfs com.delphix:hole_birth feature support

[PATCH 3/5] zfs com.delphix:hole_birth feature support

[Toomas Soome]

---
 grub-core/fs/zfs/zfs.c |    6 ++++--
 include/grub/zfs/spa.h |    4 +++-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/grub-core/fs/zfs/zfs.c b/grub-core/fs/zfs/zfs.c
index 2689986..a731c3d 100644
--- a/grub-core/fs/zfs/zfs.c
+++ b/grub-core/fs/zfs/zfs.c
@@ -280,7 +280,9 @@ grub_crypto_cipher_handle_t (*grub_zfs_load_key) (const struct grub_zfs_key *key
  */
 #define MAX_SUPPORTED_FEATURE_STRLEN 50
 static const char *spa_feature_names[] = {
-  "org.illumos:lz4_compress",NULL
+  "org.illumos:lz4_compress",
+  "com.delphix:hole_birth",
+  NULL
 };
 
 static int
@@ -1751,7 +1753,7 @@ zio_read_gang (blkptr_t * bp, grub_zfs_endian_t endian, dva_t * dva, void *buf,
 
   for (i = 0; i < SPA_GBH_NBLKPTRS; i++)
     {
-      if (zio_gb->zg_blkptr[i].blk_birth == 0)
+      if (BP_IS_HOLE(&zio_gb->zg_blkptr[i]))
 	continue;
 
       err = zio_read_data (&zio_gb->zg_blkptr[i], endian, buf, data);
diff --git a/include/grub/zfs/spa.h b/include/grub/zfs/spa.h
index 7edb8ab..df43b6b 100644
--- a/include/grub/zfs/spa.h
+++ b/include/grub/zfs/spa.h
@@ -279,7 +279,9 @@ typedef struct blkptr {
 
 #define	BP_IDENTITY(bp)		(&(bp)->blk_dva[0])
 #define	BP_IS_GANG(bp)		DVA_GET_GANG(BP_IDENTITY(bp))
-#define	BP_IS_HOLE(bp)		((bp)->blk_birth == 0)
+#define	DVA_IS_EMPTY(dva)	((dva)->dva_word[0] == 0ULL && \
+				(dva)->dva_word[1] == 0ULL)
+#define	BP_IS_HOLE(bp)		DVA_IS_EMPTY(BP_IDENTITY(bp))
 
 /* BP_IS_RAIDZ(bp) assumes no block compression */
 #define	BP_IS_RAIDZ(bp)		(DVA_GET_ASIZE(&(bp)->blk_dva[0]) > \
-- 
1.7.9.2

Re: [PATCH 3/5] zfs com.delphix:hole_birth feature support

[Andrei Borzenkov]

В Thu, 16 Apr 2015 08:22:08 +0300
Toomas Soome <tsoome@me.com> пишет:

This really needs better explanation. Otherwise this looks like either
old code was broken to start with and it is a bug fix or new code needs
some conditionals on new feature.

> 
> ---
>  grub-core/fs/zfs/zfs.c |    6 ++++--
>  include/grub/zfs/spa.h |    4 +++-
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/grub-core/fs/zfs/zfs.c b/grub-core/fs/zfs/zfs.c
> index 2689986..a731c3d 100644
> --- a/grub-core/fs/zfs/zfs.c
> +++ b/grub-core/fs/zfs/zfs.c
> @@ -280,7 +280,9 @@ grub_crypto_cipher_handle_t (*grub_zfs_load_key) (const struct grub_zfs_key *key
>   */
>  #define MAX_SUPPORTED_FEATURE_STRLEN 50
>  static const char *spa_feature_names[] = {
> -  "org.illumos:lz4_compress",NULL
> +  "org.illumos:lz4_compress",
> +  "com.delphix:hole_birth",
> +  NULL
>  };
>  
>  static int
> @@ -1751,7 +1753,7 @@ zio_read_gang (blkptr_t * bp, grub_zfs_endian_t endian, dva_t * dva, void *buf,
>  
>    for (i = 0; i < SPA_GBH_NBLKPTRS; i++)
>      {
> -      if (zio_gb->zg_blkptr[i].blk_birth == 0)
> +      if (BP_IS_HOLE(&zio_gb->zg_blkptr[i]))
>  	continue;
>  
>        err = zio_read_data (&zio_gb->zg_blkptr[i], endian, buf, data);
> diff --git a/include/grub/zfs/spa.h b/include/grub/zfs/spa.h
> index 7edb8ab..df43b6b 100644
> --- a/include/grub/zfs/spa.h
> +++ b/include/grub/zfs/spa.h
> @@ -279,7 +279,9 @@ typedef struct blkptr {
>  
>  #define	BP_IDENTITY(bp)		(&(bp)->blk_dva[0])
>  #define	BP_IS_GANG(bp)		DVA_GET_GANG(BP_IDENTITY(bp))
> -#define	BP_IS_HOLE(bp)		((bp)->blk_birth == 0)
> +#define	DVA_IS_EMPTY(dva)	((dva)->dva_word[0] == 0ULL && \
> +				(dva)->dva_word[1] == 0ULL)
> +#define	BP_IS_HOLE(bp)		DVA_IS_EMPTY(BP_IDENTITY(bp))
>  
>  /* BP_IS_RAIDZ(bp) assumes no block compression */
>  #define	BP_IS_RAIDZ(bp)		(DVA_GET_ASIZE(&(bp)->blk_dva[0]) > \

Re: [PATCH 3/5] zfs com.delphix:hole_birth feature support

[Toomas Soome]

the features in openzfs have different effects, some affect only writes and are therefore read only compatible - such feature does not need any changes for reads, this feature is not read only compatible and therefore, once this feature is enabled, reader code must be changed accordingly.

so, what they did with hole_birth was they started to insert block birth timestamps for blocks being released to make it possible to track such blocks while doing zfs send (when block has birth time, you know its place on timeline of snapshots). as old reader code was relying on blk_birth == 0 to detect an hole, after hole_birth is enabled, hole blk_birth is not 0 any more, so the solution is to check if DVA pointers are zero instead. and thats exactly what its about.  

reference:
https://github.com/illumos/illumos-gate/commit/43466aae47bfcd2ad9bf501faec8e75c08095e4f 



> On 19.04.2015, at 17:11, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
> 
> В Thu, 16 Apr 2015 08:22:08 +0300
> Toomas Soome <tsoome@me.com> пишет:
> 
> This really needs better explanation. Otherwise this looks like either
> old code was broken to start with and it is a bug fix or new code needs
> some conditionals on new feature.
> 
>> 
>> ---
>> grub-core/fs/zfs/zfs.c |    6 ++++--
>> include/grub/zfs/spa.h |    4 +++-
>> 2 files changed, 7 insertions(+), 3 deletions(-)
>> 
>> diff --git a/grub-core/fs/zfs/zfs.c b/grub-core/fs/zfs/zfs.c
>> index 2689986..a731c3d 100644
>> --- a/grub-core/fs/zfs/zfs.c
>> +++ b/grub-core/fs/zfs/zfs.c
>> @@ -280,7 +280,9 @@ grub_crypto_cipher_handle_t (*grub_zfs_load_key) (const struct grub_zfs_key *key
>>  */
>> #define MAX_SUPPORTED_FEATURE_STRLEN 50
>> static const char *spa_feature_names[] = {
>> -  "org.illumos:lz4_compress",NULL
>> +  "org.illumos:lz4_compress",
>> +  "com.delphix:hole_birth",
>> +  NULL
>> };
>> 
>> static int
>> @@ -1751,7 +1753,7 @@ zio_read_gang (blkptr_t * bp, grub_zfs_endian_t endian, dva_t * dva, void *buf,
>> 
>>   for (i = 0; i < SPA_GBH_NBLKPTRS; i++)
>>     {
>> -      if (zio_gb->zg_blkptr[i].blk_birth == 0)
>> +      if (BP_IS_HOLE(&zio_gb->zg_blkptr[i]))
>> 	continue;
>> 
>>       err = zio_read_data (&zio_gb->zg_blkptr[i], endian, buf, data);
>> diff --git a/include/grub/zfs/spa.h b/include/grub/zfs/spa.h
>> index 7edb8ab..df43b6b 100644
>> --- a/include/grub/zfs/spa.h
>> +++ b/include/grub/zfs/spa.h
>> @@ -279,7 +279,9 @@ typedef struct blkptr {
>> 
>> #define	BP_IDENTITY(bp)		(&(bp)->blk_dva[0])
>> #define	BP_IS_GANG(bp)		DVA_GET_GANG(BP_IDENTITY(bp))
>> -#define	BP_IS_HOLE(bp)		((bp)->blk_birth == 0)
>> +#define	DVA_IS_EMPTY(dva)	((dva)->dva_word[0] == 0ULL && \
>> +				(dva)->dva_word[1] == 0ULL)
>> +#define	BP_IS_HOLE(bp)		DVA_IS_EMPTY(BP_IDENTITY(bp))
>> 
>> /* BP_IS_RAIDZ(bp) assumes no block compression */
>> #define	BP_IS_RAIDZ(bp)		(DVA_GET_ASIZE(&(bp)->blk_dva[0]) > \
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 3/5] zfs com.delphix:hole_birth feature support

[Andrei Borzenkov]

В Sun, 19 Apr 2015 17:40:16 +0300
Toomas Soome <tsoome@me.com> пишет:

> 
> the features in openzfs have different effects, some affect only writes and are therefore read only compatible - such feature does not need any changes for reads, this feature is not read only compatible and therefore, once this feature is enabled, reader code must be changed accordingly.
> 
> so, what they did with hole_birth was they started to insert block birth timestamps for blocks being released to make it possible to track such blocks while doing zfs send (when block has birth time, you know its place on timeline of snapshots). as old reader code was relying on blk_birth == 0 to detect an hole, after hole_birth is enabled, hole blk_birth is not 0 any more, so the solution is to check if DVA pointers are zero instead. and thats exactly what its about.  

And if hole_birth is *not* enabled on a filesystem we are reading?
Should not old code be used in this case?

> 
> reference:
> https://github.com/illumos/illumos-gate/commit/43466aae47bfcd2ad9bf501faec8e75c08095e4f 
> 
> 
> 
> > On 19.04.2015, at 17:11, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
> > 
> > В Thu, 16 Apr 2015 08:22:08 +0300
> > Toomas Soome <tsoome@me.com> пишет:
> > 
> > This really needs better explanation. Otherwise this looks like either
> > old code was broken to start with and it is a bug fix or new code needs
> > some conditionals on new feature.
> > 
> >> 
> >> ---
> >> grub-core/fs/zfs/zfs.c |    6 ++++--
> >> include/grub/zfs/spa.h |    4 +++-
> >> 2 files changed, 7 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/grub-core/fs/zfs/zfs.c b/grub-core/fs/zfs/zfs.c
> >> index 2689986..a731c3d 100644
> >> --- a/grub-core/fs/zfs/zfs.c
> >> +++ b/grub-core/fs/zfs/zfs.c
> >> @@ -280,7 +280,9 @@ grub_crypto_cipher_handle_t (*grub_zfs_load_key) (const struct grub_zfs_key *key
> >>  */
> >> #define MAX_SUPPORTED_FEATURE_STRLEN 50
> >> static const char *spa_feature_names[] = {
> >> -  "org.illumos:lz4_compress",NULL
> >> +  "org.illumos:lz4_compress",
> >> +  "com.delphix:hole_birth",
> >> +  NULL
> >> };
> >> 
> >> static int
> >> @@ -1751,7 +1753,7 @@ zio_read_gang (blkptr_t * bp, grub_zfs_endian_t endian, dva_t * dva, void *buf,
> >> 
> >>   for (i = 0; i < SPA_GBH_NBLKPTRS; i++)
> >>     {
> >> -      if (zio_gb->zg_blkptr[i].blk_birth == 0)
> >> +      if (BP_IS_HOLE(&zio_gb->zg_blkptr[i]))
> >> 	continue;
> >> 
> >>       err = zio_read_data (&zio_gb->zg_blkptr[i], endian, buf, data);
> >> diff --git a/include/grub/zfs/spa.h b/include/grub/zfs/spa.h
> >> index 7edb8ab..df43b6b 100644
> >> --- a/include/grub/zfs/spa.h
> >> +++ b/include/grub/zfs/spa.h
> >> @@ -279,7 +279,9 @@ typedef struct blkptr {
> >> 
> >> #define	BP_IDENTITY(bp)		(&(bp)->blk_dva[0])
> >> #define	BP_IS_GANG(bp)		DVA_GET_GANG(BP_IDENTITY(bp))
> >> -#define	BP_IS_HOLE(bp)		((bp)->blk_birth == 0)
> >> +#define	DVA_IS_EMPTY(dva)	((dva)->dva_word[0] == 0ULL && \
> >> +				(dva)->dva_word[1] == 0ULL)
> >> +#define	BP_IS_HOLE(bp)		DVA_IS_EMPTY(BP_IDENTITY(bp))
> >> 
> >> /* BP_IS_RAIDZ(bp) assumes no block compression */
> >> #define	BP_IS_RAIDZ(bp)		(DVA_GET_ASIZE(&(bp)->blk_dva[0]) > \
> > 
> > 
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 3/5] zfs com.delphix:hole_birth feature support

[Toomas Soome]

> On 19.04.2015, at 17:51, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
> 
> В Sun, 19 Apr 2015 17:40:16 +0300
> Toomas Soome <tsoome@me.com> пишет:
> 
>> 
>> the features in openzfs have different effects, some affect only writes and are therefore read only compatible - such feature does not need any changes for reads, this feature is not read only compatible and therefore, once this feature is enabled, reader code must be changed accordingly.
>> 
>> so, what they did with hole_birth was they started to insert block birth timestamps for blocks being released to make it possible to track such blocks while doing zfs send (when block has birth time, you know its place on timeline of snapshots). as old reader code was relying on blk_birth == 0 to detect an hole, after hole_birth is enabled, hole blk_birth is not 0 any more, so the solution is to check if DVA pointers are zero instead. and thats exactly what its about.  
> 
> And if hole_birth is *not* enabled on a filesystem we are reading?
> Should not old code be used in this case?


not really, as for holes, DVA pointers are  zero anyhow. I guess the original implementation used blk_birth to save extra compare - so the new read code is compatible with old data.


> 
>> 
>> reference:
>> https://github.com/illumos/illumos-gate/commit/43466aae47bfcd2ad9bf501faec8e75c08095e4f 
>> 
>> 
>> 
>>> On 19.04.2015, at 17:11, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
>>> 
>>> В Thu, 16 Apr 2015 08:22:08 +0300
>>> Toomas Soome <tsoome@me.com> пишет:
>>> 
>>> This really needs better explanation. Otherwise this looks like either
>>> old code was broken to start with and it is a bug fix or new code needs
>>> some conditionals on new feature.
>>> 
>>>> 
>>>> ---
>>>> grub-core/fs/zfs/zfs.c |    6 ++++--
>>>> include/grub/zfs/spa.h |    4 +++-
>>>> 2 files changed, 7 insertions(+), 3 deletions(-)
>>>> 
>>>> diff --git a/grub-core/fs/zfs/zfs.c b/grub-core/fs/zfs/zfs.c
>>>> index 2689986..a731c3d 100644
>>>> --- a/grub-core/fs/zfs/zfs.c
>>>> +++ b/grub-core/fs/zfs/zfs.c
>>>> @@ -280,7 +280,9 @@ grub_crypto_cipher_handle_t (*grub_zfs_load_key) (const struct grub_zfs_key *key
>>>> */
>>>> #define MAX_SUPPORTED_FEATURE_STRLEN 50
>>>> static const char *spa_feature_names[] = {
>>>> -  "org.illumos:lz4_compress",NULL
>>>> +  "org.illumos:lz4_compress",
>>>> +  "com.delphix:hole_birth",
>>>> +  NULL
>>>> };
>>>> 
>>>> static int
>>>> @@ -1751,7 +1753,7 @@ zio_read_gang (blkptr_t * bp, grub_zfs_endian_t endian, dva_t * dva, void *buf,
>>>> 
>>>>  for (i = 0; i < SPA_GBH_NBLKPTRS; i++)
>>>>    {
>>>> -      if (zio_gb->zg_blkptr[i].blk_birth == 0)
>>>> +      if (BP_IS_HOLE(&zio_gb->zg_blkptr[i]))
>>>> 	continue;
>>>> 
>>>>      err = zio_read_data (&zio_gb->zg_blkptr[i], endian, buf, data);
>>>> diff --git a/include/grub/zfs/spa.h b/include/grub/zfs/spa.h
>>>> index 7edb8ab..df43b6b 100644
>>>> --- a/include/grub/zfs/spa.h
>>>> +++ b/include/grub/zfs/spa.h
>>>> @@ -279,7 +279,9 @@ typedef struct blkptr {
>>>> 
>>>> #define	BP_IDENTITY(bp)		(&(bp)->blk_dva[0])
>>>> #define	BP_IS_GANG(bp)		DVA_GET_GANG(BP_IDENTITY(bp))
>>>> -#define	BP_IS_HOLE(bp)		((bp)->blk_birth == 0)
>>>> +#define	DVA_IS_EMPTY(dva)	((dva)->dva_word[0] == 0ULL && \
>>>> +				(dva)->dva_word[1] == 0ULL)
>>>> +#define	BP_IS_HOLE(bp)		DVA_IS_EMPTY(BP_IDENTITY(bp))
>>>> 
>>>> /* BP_IS_RAIDZ(bp) assumes no block compression */
>>>> #define	BP_IS_RAIDZ(bp)		(DVA_GET_ASIZE(&(bp)->blk_dva[0]) > \
>>> 
>>> 
>>> _______________________________________________
>>> Grub-devel mailing list
>>> Grub-devel@gnu.org
>>> https://lists.gnu.org/mailman/listinfo/grub-devel
>> 
>> 
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 3/5] zfs com.delphix:hole_birth feature support

[Andrei Borzenkov]

В Sun, 19 Apr 2015 17:57:04 +0300
Toomas Soome <tsoome@me.com> пишет:

> 
> > On 19.04.2015, at 17:51, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
> > 
> > В Sun, 19 Apr 2015 17:40:16 +0300
> > Toomas Soome <tsoome@me.com> пишет:
> > 
> >> 
> >> the features in openzfs have different effects, some affect only writes and are therefore read only compatible - such feature does not need any changes for reads, this feature is not read only compatible and therefore, once this feature is enabled, reader code must be changed accordingly.
> >> 
> >> so, what they did with hole_birth was they started to insert block birth timestamps for blocks being released to make it possible to track such blocks while doing zfs send (when block has birth time, you know its place on timeline of snapshots). as old reader code was relying on blk_birth == 0 to detect an hole, after hole_birth is enabled, hole blk_birth is not 0 any more, so the solution is to check if DVA pointers are zero instead. and thats exactly what its about.  
> > 
> > And if hole_birth is *not* enabled on a filesystem we are reading?
> > Should not old code be used in this case?
> 
> 
> not really, as for holes, DVA pointers are  zero anyhow. I guess the original implementation used blk_birth to save extra compare - so the new read code is compatible with old data.

Committed with explanatory text for non-insiders.

> 
> 
> > 
> >> 
> >> reference:
> >> https://github.com/illumos/illumos-gate/commit/43466aae47bfcd2ad9bf501faec8e75c08095e4f 
> >> 
> >> 
> >> 
> >>> On 19.04.2015, at 17:11, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
> >>> 
> >>> В Thu, 16 Apr 2015 08:22:08 +0300
> >>> Toomas Soome <tsoome@me.com> пишет:
> >>> 
> >>> This really needs better explanation. Otherwise this looks like either
> >>> old code was broken to start with and it is a bug fix or new code needs
> >>> some conditionals on new feature.
> >>> 
> >>>> 
> >>>> ---
> >>>> grub-core/fs/zfs/zfs.c |    6 ++++--
> >>>> include/grub/zfs/spa.h |    4 +++-
> >>>> 2 files changed, 7 insertions(+), 3 deletions(-)
> >>>> 
> >>>> diff --git a/grub-core/fs/zfs/zfs.c b/grub-core/fs/zfs/zfs.c
> >>>> index 2689986..a731c3d 100644
> >>>> --- a/grub-core/fs/zfs/zfs.c
> >>>> +++ b/grub-core/fs/zfs/zfs.c
> >>>> @@ -280,7 +280,9 @@ grub_crypto_cipher_handle_t (*grub_zfs_load_key) (const struct grub_zfs_key *key
> >>>> */
> >>>> #define MAX_SUPPORTED_FEATURE_STRLEN 50
> >>>> static const char *spa_feature_names[] = {
> >>>> -  "org.illumos:lz4_compress",NULL
> >>>> +  "org.illumos:lz4_compress",
> >>>> +  "com.delphix:hole_birth",
> >>>> +  NULL
> >>>> };
> >>>> 
> >>>> static int
> >>>> @@ -1751,7 +1753,7 @@ zio_read_gang (blkptr_t * bp, grub_zfs_endian_t endian, dva_t * dva, void *buf,
> >>>> 
> >>>>  for (i = 0; i < SPA_GBH_NBLKPTRS; i++)
> >>>>    {
> >>>> -      if (zio_gb->zg_blkptr[i].blk_birth == 0)
> >>>> +      if (BP_IS_HOLE(&zio_gb->zg_blkptr[i]))
> >>>> 	continue;
> >>>> 
> >>>>      err = zio_read_data (&zio_gb->zg_blkptr[i], endian, buf, data);
> >>>> diff --git a/include/grub/zfs/spa.h b/include/grub/zfs/spa.h
> >>>> index 7edb8ab..df43b6b 100644
> >>>> --- a/include/grub/zfs/spa.h
> >>>> +++ b/include/grub/zfs/spa.h
> >>>> @@ -279,7 +279,9 @@ typedef struct blkptr {
> >>>> 
> >>>> #define	BP_IDENTITY(bp)		(&(bp)->blk_dva[0])
> >>>> #define	BP_IS_GANG(bp)		DVA_GET_GANG(BP_IDENTITY(bp))
> >>>> -#define	BP_IS_HOLE(bp)		((bp)->blk_birth == 0)
> >>>> +#define	DVA_IS_EMPTY(dva)	((dva)->dva_word[0] == 0ULL && \
> >>>> +				(dva)->dva_word[1] == 0ULL)
> >>>> +#define	BP_IS_HOLE(bp)		DVA_IS_EMPTY(BP_IDENTITY(bp))
> >>>> 
> >>>> /* BP_IS_RAIDZ(bp) assumes no block compression */
> >>>> #define	BP_IS_RAIDZ(bp)		(DVA_GET_ASIZE(&(bp)->blk_dva[0]) > \
> >>> 
> >>> 
> >>> _______________________________________________
> >>> Grub-devel mailing list
> >>> Grub-devel@gnu.org
> >>> https://lists.gnu.org/mailman/listinfo/grub-devel
> >> 
> >> 
> >> _______________________________________________
> >> Grub-devel mailing list
> >> Grub-devel@gnu.org
> >> https://lists.gnu.org/mailman/listinfo/grub-devel
> 

Re: [PATCH 3/5] zfs com.delphix:hole_birth feature support

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 4458 bytes --]

Thank you Toomas for patch and Andrey for reviews.
On 03.05.2015 17:15, Andrei Borzenkov wrote:
> В Sun, 19 Apr 2015 17:57:04 +0300
> Toomas Soome <tsoome@me.com> пишет:
> 
>>
>>> On 19.04.2015, at 17:51, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
>>>
>>> В Sun, 19 Apr 2015 17:40:16 +0300
>>> Toomas Soome <tsoome@me.com> пишет:
>>>
>>>>
>>>> the features in openzfs have different effects, some affect only writes and are therefore read only compatible - such feature does not need any changes for reads, this feature is not read only compatible and therefore, once this feature is enabled, reader code must be changed accordingly.
>>>>
>>>> so, what they did with hole_birth was they started to insert block birth timestamps for blocks being released to make it possible to track such blocks while doing zfs send (when block has birth time, you know its place on timeline of snapshots). as old reader code was relying on blk_birth == 0 to detect an hole, after hole_birth is enabled, hole blk_birth is not 0 any more, so the solution is to check if DVA pointers are zero instead. and thats exactly what its about.  
>>>
>>> And if hole_birth is *not* enabled on a filesystem we are reading?
>>> Should not old code be used in this case?
>>
>>
>> not really, as for holes, DVA pointers are  zero anyhow. I guess the original implementation used blk_birth to save extra compare - so the new read code is compatible with old data.
> 
> Committed with explanatory text for non-insiders.
> 
>>
>>
>>>
>>>>
>>>> reference:
>>>> https://github.com/illumos/illumos-gate/commit/43466aae47bfcd2ad9bf501faec8e75c08095e4f 
>>>>
>>>>
>>>>
>>>>> On 19.04.2015, at 17:11, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
>>>>>
>>>>> В Thu, 16 Apr 2015 08:22:08 +0300
>>>>> Toomas Soome <tsoome@me.com> пишет:
>>>>>
>>>>> This really needs better explanation. Otherwise this looks like either
>>>>> old code was broken to start with and it is a bug fix or new code needs
>>>>> some conditionals on new feature.
>>>>>
>>>>>>
>>>>>> ---
>>>>>> grub-core/fs/zfs/zfs.c |    6 ++++--
>>>>>> include/grub/zfs/spa.h |    4 +++-
>>>>>> 2 files changed, 7 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/grub-core/fs/zfs/zfs.c b/grub-core/fs/zfs/zfs.c
>>>>>> index 2689986..a731c3d 100644
>>>>>> --- a/grub-core/fs/zfs/zfs.c
>>>>>> +++ b/grub-core/fs/zfs/zfs.c
>>>>>> @@ -280,7 +280,9 @@ grub_crypto_cipher_handle_t (*grub_zfs_load_key) (const struct grub_zfs_key *key
>>>>>> */
>>>>>> #define MAX_SUPPORTED_FEATURE_STRLEN 50
>>>>>> static const char *spa_feature_names[] = {
>>>>>> -  "org.illumos:lz4_compress",NULL
>>>>>> +  "org.illumos:lz4_compress",
>>>>>> +  "com.delphix:hole_birth",
>>>>>> +  NULL
>>>>>> };
>>>>>>
>>>>>> static int
>>>>>> @@ -1751,7 +1753,7 @@ zio_read_gang (blkptr_t * bp, grub_zfs_endian_t endian, dva_t * dva, void *buf,
>>>>>>
>>>>>>  for (i = 0; i < SPA_GBH_NBLKPTRS; i++)
>>>>>>    {
>>>>>> -      if (zio_gb->zg_blkptr[i].blk_birth == 0)
>>>>>> +      if (BP_IS_HOLE(&zio_gb->zg_blkptr[i]))
>>>>>> 	continue;
>>>>>>
>>>>>>      err = zio_read_data (&zio_gb->zg_blkptr[i], endian, buf, data);
>>>>>> diff --git a/include/grub/zfs/spa.h b/include/grub/zfs/spa.h
>>>>>> index 7edb8ab..df43b6b 100644
>>>>>> --- a/include/grub/zfs/spa.h
>>>>>> +++ b/include/grub/zfs/spa.h
>>>>>> @@ -279,7 +279,9 @@ typedef struct blkptr {
>>>>>>
>>>>>> #define	BP_IDENTITY(bp)		(&(bp)->blk_dva[0])
>>>>>> #define	BP_IS_GANG(bp)		DVA_GET_GANG(BP_IDENTITY(bp))
>>>>>> -#define	BP_IS_HOLE(bp)		((bp)->blk_birth == 0)
>>>>>> +#define	DVA_IS_EMPTY(dva)	((dva)->dva_word[0] == 0ULL && \
>>>>>> +				(dva)->dva_word[1] == 0ULL)
>>>>>> +#define	BP_IS_HOLE(bp)		DVA_IS_EMPTY(BP_IDENTITY(bp))
>>>>>>
>>>>>> /* BP_IS_RAIDZ(bp) assumes no block compression */
>>>>>> #define	BP_IS_RAIDZ(bp)		(DVA_GET_ASIZE(&(bp)->blk_dva[0]) > \
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> Grub-devel mailing list
>>>>> Grub-devel@gnu.org
>>>>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>>>
>>>>
>>>> _______________________________________________
>>>> Grub-devel mailing list
>>>> Grub-devel@gnu.org
>>>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]