* [PATCH 3/5] zfs com.delphix:hole_birth feature support @ 2015-04-16 5:22 Toomas Soome 2015-04-19 14:11 ` Andrei Borzenkov 0 siblings, 1 reply; 7+ messages in thread From: Toomas Soome @ 2015-04-16 5:22 UTC (permalink / raw) To: The development of GNU GRUB --- 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 3/5] zfs com.delphix:hole_birth feature support 2015-04-16 5:22 [PATCH 3/5] zfs com.delphix:hole_birth feature support Toomas Soome @ 2015-04-19 14:11 ` Andrei Borzenkov 2015-04-19 14:40 ` Toomas Soome 0 siblings, 1 reply; 7+ messages in thread From: Andrei Borzenkov @ 2015-04-19 14:11 UTC (permalink / raw) To: Toomas Soome; +Cc: The development of GNU GRUB В 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]) > \ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 3/5] zfs com.delphix:hole_birth feature support 2015-04-19 14:11 ` Andrei Borzenkov @ 2015-04-19 14:40 ` Toomas Soome 2015-04-19 14:51 ` Andrei Borzenkov 0 siblings, 1 reply; 7+ messages in thread From: Toomas Soome @ 2015-04-19 14:40 UTC (permalink / raw) To: The development of GNU GRUB 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 3/5] zfs com.delphix:hole_birth feature support 2015-04-19 14:40 ` Toomas Soome @ 2015-04-19 14:51 ` Andrei Borzenkov 2015-04-19 14:57 ` Toomas Soome 0 siblings, 1 reply; 7+ messages in thread From: Andrei Borzenkov @ 2015-04-19 14:51 UTC (permalink / raw) To: Toomas Soome; +Cc: The development of GNU GRUB В 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 3/5] zfs com.delphix:hole_birth feature support 2015-04-19 14:51 ` Andrei Borzenkov @ 2015-04-19 14:57 ` Toomas Soome 2015-05-03 15:15 ` Andrei Borzenkov 0 siblings, 1 reply; 7+ messages in thread From: Toomas Soome @ 2015-04-19 14:57 UTC (permalink / raw) To: Andrei Borzenkov; +Cc: The development of GNU GRUB > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 3/5] zfs com.delphix:hole_birth feature support 2015-04-19 14:57 ` Toomas Soome @ 2015-05-03 15:15 ` Andrei Borzenkov 2015-05-07 9:22 ` Vladimir 'φ-coder/phcoder' Serbinenko 0 siblings, 1 reply; 7+ messages in thread From: Andrei Borzenkov @ 2015-05-03 15:15 UTC (permalink / raw) To: Toomas Soome; +Cc: The development of GNU GRUB В 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 3/5] zfs com.delphix:hole_birth feature support 2015-05-03 15:15 ` Andrei Borzenkov @ 2015-05-07 9:22 ` Vladimir 'φ-coder/phcoder' Serbinenko 0 siblings, 0 replies; 7+ messages in thread From: Vladimir 'φ-coder/phcoder' Serbinenko @ 2015-05-07 9:22 UTC (permalink / raw) To: The development of GNU GRUB [-- 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 --] ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-05-07 9:22 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-04-16 5:22 [PATCH 3/5] zfs com.delphix:hole_birth feature support Toomas Soome 2015-04-19 14:11 ` Andrei Borzenkov 2015-04-19 14:40 ` Toomas Soome 2015-04-19 14:51 ` Andrei Borzenkov 2015-04-19 14:57 ` Toomas Soome 2015-05-03 15:15 ` Andrei Borzenkov 2015-05-07 9:22 ` Vladimir 'φ-coder/phcoder' Serbinenko
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.